[PATCH] PINCTRL: SiRF: add CSR SiRFprimaII GPIO driver depending on pinmux APIs

[PATCH] PINCTRL: SiRF: add CSR SiRFprimaII GPIO driver depending on pinmux APIs

[Barry Song]

From: Barry Song <Baohua.Song@csr.com>

In SiRFprimaII, Each GPIO pin can be configured as input or output
independently. If a GPIO is configured as input, it can also be
enabled as an interrupt source (either edge or level triggered).

These pins must be either MUXed as GPIO or other function pads. So
this drivers depend on pinctrl_request_gpio() API pinctrl-sirf.c
implements.

Signed-off-by: Yuping Luo <yuping.luo@csr.com>
Signed-off-by: Barry Song <Baohua.Song@csr.com>
---
 arch/arm/boot/dts/prima2-cb.dts          |    1 +
 arch/arm/mach-prima2/include/mach/gpio.h |    1 +
 drivers/pinctrl/Makefile                 |    2 +-
 drivers/pinctrl/gpio-sirf.c              |  458 ++++++++++++++++++++++++++++++
 4 files changed, 461 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/mach-prima2/include/mach/gpio.h
 create mode 100644 drivers/pinctrl/gpio-sirf.c

diff --git a/arch/arm/boot/dts/prima2-cb.dts b/arch/arm/boot/dts/prima2-cb.dts
index 34ae3a6..4245306 100644
--- a/arch/arm/boot/dts/prima2-cb.dts
+++ b/arch/arm/boot/dts/prima2-cb.dts
@@ -284,6 +284,7 @@
 				#interrupt-cells = <2>;
 				compatible = "sirf,prima2-gpio-pinmux";
 				reg = <0xb0120000 0x10000>;
+				interrupts = <43 44 45 46 47>;
 				gpio-controller;
 				interrupt-controller;
 			};
diff --git a/arch/arm/mach-prima2/include/mach/gpio.h b/arch/arm/mach-prima2/include/mach/gpio.h
new file mode 100644
index 0000000..40a8c17
--- /dev/null
+++ b/arch/arm/mach-prima2/include/mach/gpio.h
@@ -0,0 +1 @@
+/* empty */
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 515e32f..90b699b 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -20,7 +20,7 @@ obj-$(CONFIG_PINCTRL_IMX23)	+= pinctrl-imx23.o
 obj-$(CONFIG_PINCTRL_IMX28)	+= pinctrl-imx28.o
 obj-$(CONFIG_PINCTRL_PXA168)	+= pinctrl-pxa168.o
 obj-$(CONFIG_PINCTRL_PXA910)	+= pinctrl-pxa910.o
-obj-$(CONFIG_PINCTRL_SIRF)	+= pinctrl-sirf.o
+obj-$(CONFIG_PINCTRL_SIRF)	+= pinctrl-sirf.o gpio-sirf.o
 obj-$(CONFIG_PINCTRL_TEGRA)	+= pinctrl-tegra.o
 obj-$(CONFIG_PINCTRL_TEGRA20)	+= pinctrl-tegra20.o
 obj-$(CONFIG_PINCTRL_TEGRA30)	+= pinctrl-tegra30.o
diff --git a/drivers/pinctrl/gpio-sirf.c b/drivers/pinctrl/gpio-sirf.c
new file mode 100644
index 0000000..3cc696d
--- /dev/null
+++ b/drivers/pinctrl/gpio-sirf.c
@@ -0,0 +1,458 @@
+/*
+ * GPIO controller driver for CSR SiRFprimaII
+ *
+ * Copyright (c) 2011 Cambridge Silicon Radio Limited, a CSR plc group company.
+ *
+ * Licensed under GPLv2 or later.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/irq.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_address.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/pinctrl/consumer.h>
+
+#define SIRFSOC_GPIO_IRQ_START     (SIRFSOC_INTENAL_IRQ_END + 1)
+
+#define SIRFSOC_GPIO_NO_OF_BANKS        5
+#define SIRFSOC_GPIO_BANK_SIZE          32
+
+#define SIRFSOC_GPIO_CTRL(g, i)			((g)*0x100 + (i)*4)
+#define SIRFSOC_GPIO_DSP_EN0			(0x80)
+#define SIRFSOC_GPIO_PAD_EN(g)			((g)*0x100 + 0x84)
+#define SIRFSOC_GPIO_INT_STATUS(g)		((g)*0x100 + 0x8C)
+
+#define SIRFSOC_GPIO_CTL_INTR_LOW_MASK		0x1
+#define SIRFSOC_GPIO_CTL_INTR_HIGH_MASK		0x2
+#define SIRFSOC_GPIO_CTL_INTR_TYPE_MASK		0x4
+#define SIRFSOC_GPIO_CTL_INTR_EN_MASK		0x8
+#define SIRFSOC_GPIO_CTL_INTR_STS_MASK		0x10
+#define SIRFSOC_GPIO_CTL_OUT_EN_MASK		0x20
+#define SIRFSOC_GPIO_CTL_DATAOUT_MASK		0x40
+#define SIRFSOC_GPIO_CTL_DATAIN_MASK		0x80
+#define SIRFSOC_GPIO_CTL_PULL_MASK		0x100
+#define SIRFSOC_GPIO_CTL_PULL_HIGH		0x200
+#define SIRFSOC_GPIO_CTL_DSP_INT		0x400
+
+#define SIRFSOC_GPIO_NUM(bank, index)	(((bank)*(32)) + (index))
+
+struct sirfsoc_gpio_bank {
+	struct of_mm_gpio_chip chip;
+	int id;
+	int irq;
+	spinlock_t lock;
+};
+
+static struct sirfsoc_gpio_bank sgpio_bank[SIRFSOC_GPIO_NO_OF_BANKS];
+
+static DEFINE_SPINLOCK(sgpio_lock);
+
+static inline struct sirfsoc_gpio_bank *sirfsoc_irq_to_bank(unsigned int irq)
+{
+	return &sgpio_bank[(irq - SIRFSOC_GPIO_IRQ_START) / SIRFSOC_GPIO_BANK_SIZE];
+}
+
+static inline int sirfsoc_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
+{
+	return SIRFSOC_GPIO_IRQ_START + (chip->base + offset);
+}
+
+static inline int sirfsoc_irq_to_offset(unsigned int irq)
+{
+	return (irq - SIRFSOC_GPIO_IRQ_START) % SIRFSOC_GPIO_BANK_SIZE;
+}
+
+static inline int sirfsoc_gpio_to_offset(unsigned int gpio)
+{
+	return gpio % SIRFSOC_GPIO_BANK_SIZE;
+}
+
+static inline struct sirfsoc_gpio_bank *sirfsoc_irqchip_to_bank(struct gpio_chip *chip)
+{
+	return container_of(to_of_mm_gpio_chip(chip), struct sirfsoc_gpio_bank, chip);
+}
+
+static void sirfsoc_gpio_irq_ack(struct irq_data *d)
+{
+	struct sirfsoc_gpio_bank *bank = sirfsoc_irq_to_bank(d->irq);
+	int idx = sirfsoc_irq_to_offset(d->irq);
+	u32 status, offset;
+	unsigned long flags;
+
+	offset = SIRFSOC_GPIO_CTRL(bank->id, idx);
+
+	spin_lock_irqsave(&sgpio_lock, flags);
+
+	status = readl(bank->chip.regs + offset);
+
+	writel(status, bank->chip.regs + offset);
+
+	spin_unlock_irqrestore(&sgpio_lock, flags);
+}
+
+static void __sirfsoc_gpio_irq_mask(unsigned int irq)
+{
+	struct sirfsoc_gpio_bank *bank = sirfsoc_irq_to_bank(irq);
+	int idx = sirfsoc_irq_to_offset(irq);
+	u32 status, offset;
+	unsigned long flags;
+
+	offset = SIRFSOC_GPIO_CTRL(bank->id, idx);
+
+	spin_lock_irqsave(&sgpio_lock, flags);
+
+	status = readl(bank->chip.regs + offset);
+	status &= ~SIRFSOC_GPIO_CTL_INTR_EN_MASK;
+	status &= ~SIRFSOC_GPIO_CTL_INTR_STS_MASK;
+	writel(status, bank->chip.regs + offset);
+
+	spin_unlock_irqrestore(&sgpio_lock, flags);
+}
+
+static void sirfsoc_gpio_irq_mask(struct irq_data *d)
+{
+	__sirfsoc_gpio_irq_mask(d->irq);
+}
+
+static void sirfsoc_gpio_irq_unmask(struct irq_data *d)
+{
+	struct sirfsoc_gpio_bank *bank = sirfsoc_irq_to_bank(d->irq);
+	int idx = sirfsoc_irq_to_offset(d->irq);
+	u32 status, offset;
+	unsigned long flags;
+
+	offset = SIRFSOC_GPIO_CTRL(bank->id, idx);
+
+	spin_lock_irqsave(&sgpio_lock, flags);
+
+	status = readl(bank->chip.regs + offset);
+	status &= ~SIRFSOC_GPIO_CTL_INTR_STS_MASK;
+	status |= SIRFSOC_GPIO_CTL_INTR_EN_MASK;
+	writel(status, bank->chip.regs + offset);
+
+	spin_unlock_irqrestore(&sgpio_lock, flags);
+}
+
+static int sirfsoc_gpio_irq_type(struct irq_data *d, unsigned type)
+{
+	struct sirfsoc_gpio_bank *bank = sirfsoc_irq_to_bank(d->irq);
+	int idx = sirfsoc_irq_to_offset(d->irq);
+	u32 status, offset;
+	unsigned long flags;
+
+	offset = SIRFSOC_GPIO_CTRL(bank->id, idx);
+
+	spin_lock_irqsave(&sgpio_lock, flags);
+
+	status = readl(bank->chip.regs + offset);
+	status &= ~SIRFSOC_GPIO_CTL_INTR_STS_MASK;
+
+	switch (type) {
+	case IRQ_TYPE_NONE:
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+		status |= (SIRFSOC_GPIO_CTL_INTR_HIGH_MASK | SIRFSOC_GPIO_CTL_INTR_TYPE_MASK);
+		status &= ~SIRFSOC_GPIO_CTL_INTR_LOW_MASK;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		status &= ~SIRFSOC_GPIO_CTL_INTR_HIGH_MASK;
+		status |= (SIRFSOC_GPIO_CTL_INTR_LOW_MASK | SIRFSOC_GPIO_CTL_INTR_TYPE_MASK);
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		status |=
+			(SIRFSOC_GPIO_CTL_INTR_HIGH_MASK | SIRFSOC_GPIO_CTL_INTR_LOW_MASK |
+			 SIRFSOC_GPIO_CTL_INTR_TYPE_MASK);
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		status &= ~(SIRFSOC_GPIO_CTL_INTR_HIGH_MASK | SIRFSOC_GPIO_CTL_INTR_TYPE_MASK);
+		status |= SIRFSOC_GPIO_CTL_INTR_LOW_MASK;
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		status |= SIRFSOC_GPIO_CTL_INTR_HIGH_MASK;
+		status &= ~(SIRFSOC_GPIO_CTL_INTR_LOW_MASK | SIRFSOC_GPIO_CTL_INTR_TYPE_MASK);
+		break;
+	}
+
+	writel(status, bank->chip.regs + offset);
+
+	spin_unlock_irqrestore(&sgpio_lock, flags);
+
+	return 0;
+}
+
+static struct irq_chip sirfsoc_irq_chip = {
+	.name = "SiRF SoC GPIO IRQ",
+	.irq_ack = sirfsoc_gpio_irq_ack,
+	.irq_mask = sirfsoc_gpio_irq_mask,
+	.irq_unmask = sirfsoc_gpio_irq_unmask,
+	.irq_set_type = sirfsoc_gpio_irq_type,
+};
+
+static void sirfsoc_gpio_handle_irq(unsigned int irq, struct irq_desc *desc)
+{
+	struct sirfsoc_gpio_bank *bank = NULL;
+	u32 status, ctrl;
+	int i, idx = 0;
+
+	for (i = 0; i < SIRFSOC_GPIO_NO_OF_BANKS; i++) {
+		if (sgpio_bank[i].irq == irq) {
+			bank = &sgpio_bank[i];
+			break;
+		}
+	}
+
+	status = readl(bank->chip.regs + SIRFSOC_GPIO_INT_STATUS(bank->id));
+	if (!status) {
+		printk(KERN_WARNING
+			"%s: gpio id %d status %#x no interrupt is flaged\n",
+			__func__, bank->id, status);
+		handle_bad_irq(irq, desc);
+		return;
+	}
+
+	while (status) {
+		ctrl = readl(bank->chip.regs + SIRFSOC_GPIO_CTRL(bank->id, idx));
+
+		/*
+		 * Here we must check whether the corresponding GPIO's interrupt
+		 * has been enabled, otherwise just skip it
+		 */
+		if ((status & 0x1) && (ctrl & SIRFSOC_GPIO_CTL_INTR_EN_MASK)) {
+			pr_debug("%s: gpio id %d idx %d happens\n",
+				__func__, bank->id, idx);
+			irq =
+				(SIRFSOC_GPIO_IRQ_START +
+				 (bank->id * SIRFSOC_GPIO_BANK_SIZE)) + idx;
+			generic_handle_irq(irq);
+		}
+
+		idx++;
+		status = status >> 1;
+	}
+}
+
+static inline void sirfsoc_gpio_set_input(struct sirfsoc_gpio_bank *bank, unsigned ctrl_offset)
+{
+	u32 status;
+	unsigned long flags;
+
+	spin_lock_irqsave(&bank->lock, flags);
+
+	status = readl(bank->chip.regs + ctrl_offset);
+	status &= ~SIRFSOC_GPIO_CTL_OUT_EN_MASK;
+	writel(status, bank->chip.regs + ctrl_offset);
+
+	spin_unlock_irqrestore(&bank->lock, flags);
+}
+
+static int sirfsoc_gpio_request(struct gpio_chip *chip, unsigned offset)
+{
+	struct sirfsoc_gpio_bank *bank = sirfsoc_irqchip_to_bank(chip);
+	unsigned long flags;
+
+	if (pinctrl_request_gpio(chip->base + offset))
+		return -ENODEV;
+
+	spin_lock_irqsave(&bank->lock, flags);
+
+	/*
+	 * default status:
+	 * set direction as input and mask irq
+	 */
+	sirfsoc_gpio_set_input(bank, SIRFSOC_GPIO_CTRL(bank->id, offset));
+	__sirfsoc_gpio_irq_mask(sirfsoc_gpio_to_irq(chip, offset));
+
+	spin_unlock_irqrestore(&bank->lock, flags);
+
+	return 0;
+}
+
+static void sirfsoc_gpio_free(struct gpio_chip *chip, unsigned offset)
+{
+	struct sirfsoc_gpio_bank *bank = sirfsoc_irqchip_to_bank(chip);
+	unsigned long flags;
+
+	spin_lock_irqsave(&bank->lock, flags);
+
+	__sirfsoc_gpio_irq_mask(sirfsoc_gpio_to_irq(chip, offset));
+	sirfsoc_gpio_set_input(bank, SIRFSOC_GPIO_CTRL(bank->id, offset));
+
+	spin_unlock_irqrestore(&bank->lock, flags);
+
+	pinctrl_free_gpio(chip->base + offset);
+}
+
+static int sirfsoc_gpio_direction_input(struct gpio_chip *chip, unsigned gpio)
+{
+	struct sirfsoc_gpio_bank *bank = sirfsoc_irqchip_to_bank(chip);
+	int idx = sirfsoc_gpio_to_offset(gpio);
+	unsigned long flags;
+	unsigned offset;
+
+	offset = SIRFSOC_GPIO_CTRL(bank->id, idx);
+
+	spin_lock_irqsave(&bank->lock, flags);
+
+	sirfsoc_gpio_set_input(bank, offset);
+
+	spin_unlock_irqrestore(&bank->lock, flags);
+
+	return 0;
+}
+
+static inline void sirfsoc_gpio_set_output(struct sirfsoc_gpio_bank *bank, unsigned offset,
+	int value)
+{
+	u32 status;
+	unsigned long flags;
+
+	spin_lock_irqsave(&bank->lock, flags);
+
+	status = readl(bank->chip.regs + offset);
+	if (value)
+		status |= SIRFSOC_GPIO_CTL_DATAOUT_MASK;
+	else
+		status &= ~SIRFSOC_GPIO_CTL_DATAOUT_MASK;
+
+	status &= ~SIRFSOC_GPIO_CTL_INTR_EN_MASK;
+	status |= SIRFSOC_GPIO_CTL_OUT_EN_MASK;
+	writel(status, bank->chip.regs + offset);
+
+	spin_unlock_irqrestore(&bank->lock, flags);
+}
+
+static int sirfsoc_gpio_direction_output(struct gpio_chip *chip, unsigned gpio, int value)
+{
+	struct sirfsoc_gpio_bank *bank = sirfsoc_irqchip_to_bank(chip);
+	int idx = sirfsoc_gpio_to_offset(gpio);
+	u32 offset;
+	unsigned long flags;
+
+	offset = SIRFSOC_GPIO_CTRL(bank->id, idx);
+
+	spin_lock_irqsave(&sgpio_lock, flags);
+
+	sirfsoc_gpio_set_output(bank, offset, value);
+
+	spin_unlock_irqrestore(&sgpio_lock, flags);
+
+	return 0;
+}
+
+static int sirfsoc_gpio_get_value(struct gpio_chip *chip, unsigned offset)
+{
+	struct sirfsoc_gpio_bank *bank = sirfsoc_irqchip_to_bank(chip);
+	u32 status;
+	unsigned long flags;
+
+	spin_lock_irqsave(&bank->lock, flags);
+
+	status = readl(bank->chip.regs + SIRFSOC_GPIO_CTRL(bank->id, offset));
+
+	spin_unlock_irqrestore(&bank->lock, flags);
+
+	return !!(status & SIRFSOC_GPIO_CTL_DATAIN_MASK);
+}
+
+static void sirfsoc_gpio_set_value(struct gpio_chip *chip, unsigned offset,
+	int value)
+{
+	struct sirfsoc_gpio_bank *bank = sirfsoc_irqchip_to_bank(chip);
+	u32 status;
+	unsigned long flags;
+
+	spin_lock_irqsave(&bank->lock, flags);
+
+	status = readl(bank->chip.regs + SIRFSOC_GPIO_CTRL(bank->id, offset));
+	if (value)
+		status |= SIRFSOC_GPIO_CTL_DATAOUT_MASK;
+	else
+		status &= ~SIRFSOC_GPIO_CTL_DATAOUT_MASK;
+	writel(status, bank->chip.regs + SIRFSOC_GPIO_CTRL(bank->id, offset));
+
+	spin_unlock_irqrestore(&bank->lock, flags);
+}
+
+static int __devinit sirfsoc_gpio_probe(struct device_node *np)
+{
+	int i, err = 0;
+	struct sirfsoc_gpio_bank *bank;
+	void *regs;
+	struct platform_device *pdev;
+
+	pdev = of_find_device_by_node(np);
+	if (!pdev)
+		return -ENODEV;
+
+	regs = of_iomap(np, 0);
+	if (!regs)
+		return -ENOMEM;
+
+	for (i = 0; i < SIRFSOC_GPIO_NO_OF_BANKS; i++) {
+		bank = &sgpio_bank[i];
+		spin_lock_init(&bank->lock);
+		bank->chip.gc.request = sirfsoc_gpio_request;
+		bank->chip.gc.free = sirfsoc_gpio_free;
+		bank->chip.gc.direction_input = sirfsoc_gpio_direction_input;
+		bank->chip.gc.get = sirfsoc_gpio_get_value;
+		bank->chip.gc.direction_output = sirfsoc_gpio_direction_output;
+		bank->chip.gc.set = sirfsoc_gpio_set_value;
+		bank->chip.gc.to_irq = sirfsoc_gpio_to_irq;
+		bank->chip.gc.base = i * SIRFSOC_GPIO_BANK_SIZE;
+		bank->chip.gc.ngpio = SIRFSOC_GPIO_BANK_SIZE;
+		bank->chip.gc.label = kstrdup(np->full_name, GFP_KERNEL);
+		bank->chip.gc.of_node = np;
+		bank->chip.regs = regs;
+		bank->id = i;
+		bank->irq = platform_get_irq(pdev, i);
+		if (bank->irq < 0) {
+			err = bank->irq;
+			goto out;
+		}
+
+		/* Call the OF gpio helper to setup and register the GPIO device */
+		err = gpiochip_add(&bank->chip.gc);
+		if (err) {
+			pr_err("%s: error in probe function with status %d\n",
+				np->full_name, err);
+			goto out;
+		}
+
+		irq_set_chained_handler(bank->irq, sirfsoc_gpio_handle_irq);
+		irq_set_chip(bank->irq, &sirfsoc_irq_chip);
+		irq_set_handler(bank->irq, handle_level_irq);
+		set_irq_flags(bank->irq, IRQF_VALID | IRQF_PROBE);
+	}
+
+out:
+	iounmap(regs);
+	return err;
+}
+
+static const struct of_device_id sgpio_of_match[] __devinitconst = {
+	{.compatible = "sirf,prima2-gpio-pinmux", },
+	{},
+};
+
+static int __init sirfsoc_gpio_init(void)
+{
+
+	struct device_node *np;
+
+	np = of_find_matching_node(NULL, sgpio_of_match);
+
+	if (!np)
+		return -ENODEV;
+
+	return sirfsoc_gpio_probe(np);
+}
+subsys_initcall(sirfsoc_gpio_init);
+
+MODULE_DESCRIPTION("SiRFSoC GPIO driver");
+MODULE_AUTHOR("Yuping Luo <yuping.luo@csr.com>, Barry Song <baohua.song@csr.com>");
+MODULE_LICENSE("GPL v2");
-- 
1.7.1



Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
More information can be found at www.csr.com. Follow CSR on Twitter at http://twitter.com/CSR_PLC and read our blog at www.csr.com/blog

[PATCH] PINCTRL: SiRF: add CSR SiRFprimaII GPIO driver depending on pinmux APIs

[Linus Walleij]

On Tue, May 22, 2012 at 9:14 AM, Barry Song <Barry.Song@csr.com> wrote:

> From: Barry Song <Baohua.Song@csr.com>
>
> In SiRFprimaII, Each GPIO pin can be configured as input or output
> independently. If a GPIO is configured as input, it can also be
> enabled as an interrupt source (either edge or level triggered).
>
> These pins must be either MUXed as GPIO or other function pads. So
> this drivers depend on pinctrl_request_gpio() API pinctrl-sirf.c
> implements.
>
> Signed-off-by: Yuping Luo <yuping.luo@csr.com>
> Signed-off-by: Barry Song <Baohua.Song@csr.com>

This is actually such a nice and clean cut that it can be put into
drivers/gpio, it's just using pinctrl, not sharing code or hardware
registers with it. (If I understand correctly.)

Let's check with Grant how he wants these things.

> +#define SIRFSOC_GPIO_IRQ_START ? ? (SIRFSOC_INTENAL_IRQ_END + 1)

This is spelled wrong (inteRnal) but where is that #define coming from
anyway? And is this really used? I think this line can be simply deleted.

> +static inline struct sirfsoc_gpio_bank *sirfsoc_irq_to_bank(unsigned int irq)
> +{
> + ? ? ? return &sgpio_bank[(irq - SIRFSOC_GPIO_IRQ_START) / SIRFSOC_GPIO_BANK_SIZE];
> +}
> +
> +static inline int sirfsoc_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
> +{
> + ? ? ? return SIRFSOC_GPIO_IRQ_START + (chip->base + offset);
> +}

Ick! Can you please use irqdomain for this?

See the recent drivers/gpio/gpio-em.c driver for an excellent example.

> +static int sirfsoc_gpio_irq_type(struct irq_data *d, unsigned type)
> +{
> + ? ? ? struct sirfsoc_gpio_bank *bank = sirfsoc_irq_to_bank(d->irq);
> + ? ? ? int idx = sirfsoc_irq_to_offset(d->irq);
> + ? ? ? u32 status, offset;

"status" is a real bad name for this variable since you're not
checking or changing any status by it. Use "val" or something.

> + ? ? ? unsigned long flags;
> +
> + ? ? ? offset = SIRFSOC_GPIO_CTRL(bank->id, idx);
> +
> + ? ? ? spin_lock_irqsave(&sgpio_lock, flags);
> +
> + ? ? ? status = readl(bank->chip.regs + offset);
> + ? ? ? status &= ~SIRFSOC_GPIO_CTL_INTR_STS_MASK;
> +
> + ? ? ? switch (type) {
> + ? ? ? case IRQ_TYPE_NONE:
> + ? ? ? ? ? ? ? break;
> + ? ? ? case IRQ_TYPE_EDGE_RISING:
> + ? ? ? ? ? ? ? status |= (SIRFSOC_GPIO_CTL_INTR_HIGH_MASK | SIRFSOC_GPIO_CTL_INTR_TYPE_MASK);
> + ? ? ? ? ? ? ? status &= ~SIRFSOC_GPIO_CTL_INTR_LOW_MASK;
> + ? ? ? ? ? ? ? break;
> + ? ? ? case IRQ_TYPE_EDGE_FALLING:
> + ? ? ? ? ? ? ? status &= ~SIRFSOC_GPIO_CTL_INTR_HIGH_MASK;
> + ? ? ? ? ? ? ? status |= (SIRFSOC_GPIO_CTL_INTR_LOW_MASK | SIRFSOC_GPIO_CTL_INTR_TYPE_MASK);
> + ? ? ? ? ? ? ? break;
> + ? ? ? case IRQ_TYPE_EDGE_BOTH:
> + ? ? ? ? ? ? ? status |=
> + ? ? ? ? ? ? ? ? ? ? ? (SIRFSOC_GPIO_CTL_INTR_HIGH_MASK | SIRFSOC_GPIO_CTL_INTR_LOW_MASK |
> + ? ? ? ? ? ? ? ? ? ? ? ?SIRFSOC_GPIO_CTL_INTR_TYPE_MASK);
> + ? ? ? ? ? ? ? break;
> + ? ? ? case IRQ_TYPE_LEVEL_LOW:
> + ? ? ? ? ? ? ? status &= ~(SIRFSOC_GPIO_CTL_INTR_HIGH_MASK | SIRFSOC_GPIO_CTL_INTR_TYPE_MASK);
> + ? ? ? ? ? ? ? status |= SIRFSOC_GPIO_CTL_INTR_LOW_MASK;
> + ? ? ? ? ? ? ? break;
> + ? ? ? case IRQ_TYPE_LEVEL_HIGH:
> + ? ? ? ? ? ? ? status |= SIRFSOC_GPIO_CTL_INTR_HIGH_MASK;
> + ? ? ? ? ? ? ? status &= ~(SIRFSOC_GPIO_CTL_INTR_LOW_MASK | SIRFSOC_GPIO_CTL_INTR_TYPE_MASK);
> + ? ? ? ? ? ? ? break;
> + ? ? ? }
> +
> + ? ? ? writel(status, bank->chip.regs + offset);
> +
> + ? ? ? spin_unlock_irqrestore(&sgpio_lock, flags);
> +
> + ? ? ? return 0;
> +}
(...)
> + ? ? ? ? ? ? ? /*
> + ? ? ? ? ? ? ? ?* Here we must check whether the corresponding GPIO's interrupt
> + ? ? ? ? ? ? ? ?* has been enabled, otherwise just skip it
> + ? ? ? ? ? ? ? ?*/
> + ? ? ? ? ? ? ? if ((status & 0x1) && (ctrl & SIRFSOC_GPIO_CTL_INTR_EN_MASK)) {
> + ? ? ? ? ? ? ? ? ? ? ? pr_debug("%s: gpio id %d idx %d happens\n",
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? __func__, bank->id, idx);
> + ? ? ? ? ? ? ? ? ? ? ? irq =
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? (SIRFSOC_GPIO_IRQ_START +
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?(bank->id * SIRFSOC_GPIO_BANK_SIZE)) + idx;

So this is where irqdomain makes things simpler.

> + ? ? ? ? ? ? ? ? ? ? ? generic_handle_irq(irq);
> + ? ? ? ? ? ? ? }
> +
> + ? ? ? ? ? ? ? idx++;
> + ? ? ? ? ? ? ? status = status >> 1;
> + ? ? ? }
> +}

> +static inline void sirfsoc_gpio_set_output(struct sirfsoc_gpio_bank *bank, unsigned offset,
> + ? ? ? int value)
> +{
> + ? ? ? u32 status;

Again what does this has to do with any status...

> +static void sirfsoc_gpio_set_value(struct gpio_chip *chip, unsigned offset,
> + ? ? ? int value)
> +{
> + ? ? ? struct sirfsoc_gpio_bank *bank = sirfsoc_irqchip_to_bank(chip);
> + ? ? ? u32 status;

Neither this.

Apart from that it looks good.

Yours,
Linus Walleij

[PATCH] PINCTRL: SiRF: add CSR SiRFprimaII GPIO driver depending on pinmux APIs

[Grant Likely]

On Thu, 24 May 2012 16:48:25 +0200, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, May 22, 2012 at 9:14 AM, Barry Song <Barry.Song@csr.com> wrote:
> 
> > From: Barry Song <Baohua.Song@csr.com>
> >
> > In SiRFprimaII, Each GPIO pin can be configured as input or output
> > independently. If a GPIO is configured as input, it can also be
> > enabled as an interrupt source (either edge or level triggered).
> >
> > These pins must be either MUXed as GPIO or other function pads. So
> > this drivers depend on pinctrl_request_gpio() API pinctrl-sirf.c
> > implements.
> >
> > Signed-off-by: Yuping Luo <yuping.luo@csr.com>
> > Signed-off-by: Barry Song <Baohua.Song@csr.com>
> 
> This is actually such a nice and clean cut that it can be put into
> drivers/gpio, it's just using pinctrl, not sharing code or hardware
> registers with it. (If I understand correctly.)
> 
> Let's check with Grant how he wants these things.

If it is a gpio controller, then it really should be in drivers/gpio.
If it is a memory mapped gpio controller, then it really should use
gpio-generic.c

> 
> > +#define SIRFSOC_GPIO_IRQ_START ?? ?? (SIRFSOC_INTENAL_IRQ_END + 1)
> 
> This is spelled wrong (inteRnal) but where is that #define coming from
> anyway? And is this really used? I think this line can be simply deleted.
> 
> > +static inline struct sirfsoc_gpio_bank *sirfsoc_irq_to_bank(unsigned int irq)
> > +{
> > + ?? ?? ?? return &sgpio_bank[(irq - SIRFSOC_GPIO_IRQ_START) / SIRFSOC_GPIO_BANK_SIZE];
> > +}
> > +
> > +static inline int sirfsoc_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
> > +{
> > + ?? ?? ?? return SIRFSOC_GPIO_IRQ_START + (chip->base + offset);
> > +}
> 
> Ick! Can you please use irqdomain for this?

Yes.

> > +static int sirfsoc_gpio_irq_type(struct irq_data *d, unsigned type)
> > +{
> > + ?? ?? ?? struct sirfsoc_gpio_bank *bank = sirfsoc_irq_to_bank(d->irq);
> > + ?? ?? ?? int idx = sirfsoc_irq_to_offset(d->irq);
> > + ?? ?? ?? u32 status, offset;
> 
> "status" is a real bad name for this variable since you're not
> checking or changing any status by it. Use "val" or something.
> 
> > + ?? ?? ?? unsigned long flags;
> > +
> > + ?? ?? ?? offset = SIRFSOC_GPIO_CTRL(bank->id, idx);
> > +
> > + ?? ?? ?? spin_lock_irqsave(&sgpio_lock, flags);
> > +
> > + ?? ?? ?? status = readl(bank->chip.regs + offset);
> > + ?? ?? ?? status &= ~SIRFSOC_GPIO_CTL_INTR_STS_MASK;
> > +
> > + ?? ?? ?? switch (type) {
> > + ?? ?? ?? case IRQ_TYPE_NONE:
> > + ?? ?? ?? ?? ?? ?? ?? break;
> > + ?? ?? ?? case IRQ_TYPE_EDGE_RISING:
> > + ?? ?? ?? ?? ?? ?? ?? status |= (SIRFSOC_GPIO_CTL_INTR_HIGH_MASK | SIRFSOC_GPIO_CTL_INTR_TYPE_MASK);
> > + ?? ?? ?? ?? ?? ?? ?? status &= ~SIRFSOC_GPIO_CTL_INTR_LOW_MASK;
> > + ?? ?? ?? ?? ?? ?? ?? break;
> > + ?? ?? ?? case IRQ_TYPE_EDGE_FALLING:
> > + ?? ?? ?? ?? ?? ?? ?? status &= ~SIRFSOC_GPIO_CTL_INTR_HIGH_MASK;
> > + ?? ?? ?? ?? ?? ?? ?? status |= (SIRFSOC_GPIO_CTL_INTR_LOW_MASK | SIRFSOC_GPIO_CTL_INTR_TYPE_MASK);
> > + ?? ?? ?? ?? ?? ?? ?? break;
> > + ?? ?? ?? case IRQ_TYPE_EDGE_BOTH:
> > + ?? ?? ?? ?? ?? ?? ?? status |=
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? (SIRFSOC_GPIO_CTL_INTR_HIGH_MASK | SIRFSOC_GPIO_CTL_INTR_LOW_MASK |
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??SIRFSOC_GPIO_CTL_INTR_TYPE_MASK);
> > + ?? ?? ?? ?? ?? ?? ?? break;
> > + ?? ?? ?? case IRQ_TYPE_LEVEL_LOW:
> > + ?? ?? ?? ?? ?? ?? ?? status &= ~(SIRFSOC_GPIO_CTL_INTR_HIGH_MASK | SIRFSOC_GPIO_CTL_INTR_TYPE_MASK);
> > + ?? ?? ?? ?? ?? ?? ?? status |= SIRFSOC_GPIO_CTL_INTR_LOW_MASK;
> > + ?? ?? ?? ?? ?? ?? ?? break;
> > + ?? ?? ?? case IRQ_TYPE_LEVEL_HIGH:
> > + ?? ?? ?? ?? ?? ?? ?? status |= SIRFSOC_GPIO_CTL_INTR_HIGH_MASK;
> > + ?? ?? ?? ?? ?? ?? ?? status &= ~(SIRFSOC_GPIO_CTL_INTR_LOW_MASK | SIRFSOC_GPIO_CTL_INTR_TYPE_MASK);
> > + ?? ?? ?? ?? ?? ?? ?? break;
> > + ?? ?? ?? }
> > +
> > + ?? ?? ?? writel(status, bank->chip.regs + offset);
> > +
> > + ?? ?? ?? spin_unlock_irqrestore(&sgpio_lock, flags);
> > +
> > + ?? ?? ?? return 0;
> > +}
> (...)
> > + ?? ?? ?? ?? ?? ?? ?? /*
> > + ?? ?? ?? ?? ?? ?? ?? ??* Here we must check whether the corresponding GPIO's interrupt
> > + ?? ?? ?? ?? ?? ?? ?? ??* has been enabled, otherwise just skip it
> > + ?? ?? ?? ?? ?? ?? ?? ??*/
> > + ?? ?? ?? ?? ?? ?? ?? if ((status & 0x1) && (ctrl & SIRFSOC_GPIO_CTL_INTR_EN_MASK)) {
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? pr_debug("%s: gpio id %d idx %d happens\n",
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? __func__, bank->id, idx);
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? irq =
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? (SIRFSOC_GPIO_IRQ_START +
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??(bank->id * SIRFSOC_GPIO_BANK_SIZE)) + idx;
> 
> So this is where irqdomain makes things simpler.
> 
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? generic_handle_irq(irq);
> > + ?? ?? ?? ?? ?? ?? ?? }
> > +
> > + ?? ?? ?? ?? ?? ?? ?? idx++;
> > + ?? ?? ?? ?? ?? ?? ?? status = status >> 1;
> > + ?? ?? ?? }
> > +}
> 
> > +static inline void sirfsoc_gpio_set_output(struct sirfsoc_gpio_bank *bank, unsigned offset,
> > + ?? ?? ?? int value)
> > +{
> > + ?? ?? ?? u32 status;
> 
> Again what does this has to do with any status...
> 
> > +static void sirfsoc_gpio_set_value(struct gpio_chip *chip, unsigned offset,
> > + ?? ?? ?? int value)
> > +{
> > + ?? ?? ?? struct sirfsoc_gpio_bank *bank = sirfsoc_irqchip_to_bank(chip);
> > + ?? ?? ?? u32 status;
> 
> Neither this.
> 
> Apart from that it looks good.
> 
> Yours,
> Linus Walleij

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

[PATCH] PINCTRL: SiRF: add CSR SiRFprimaII GPIO driver depending on pinmux APIs

[Barry Song]

Hi Linus,
Thanks!

2012/5/24 Linus Walleij <linus.walleij@linaro.org>:
> On Tue, May 22, 2012 at 9:14 AM, Barry Song <Barry.Song@csr.com> wrote:
>
>> From: Barry Song <Baohua.Song@csr.com>
>>
>> In SiRFprimaII, Each GPIO pin can be configured as input or output
>> independently. If a GPIO is configured as input, it can also be
>> enabled as an interrupt source (either edge or level triggered).
>>
>> These pins must be either MUXed as GPIO or other function pads. So
>> this drivers depend on pinctrl_request_gpio() API pinctrl-sirf.c
>> implements.
>>
>> Signed-off-by: Yuping Luo <yuping.luo@csr.com>
>> Signed-off-by: Barry Song <Baohua.Song@csr.com>
>
> This is actually such a nice and clean cut that it can be put into
> drivers/gpio, it's just using pinctrl, not sharing code or hardware
> registers with it. (If I understand correctly.)

it is actually one pinmux/gpio/irq controller. as you see, i am using
the same dt node with pinctrl-sirf.c:
static const struct of_device_id sgpio_of_match[] __devinitconst = {
       {.compatible = "sirf,prima2-gpio-pinmux", },
       {},
};

here i splitted into two files just for viewing, and at the same time,
i remap the same memory twice in pinctrl-sirf.c and gpio-sirf.c. they
share same memory area.
these codes might be in pinctrl-sirf.c directly?

>
> Let's check with Grant how he wants these things.
>
>> +#define SIRFSOC_GPIO_IRQ_START ? ? (SIRFSOC_INTENAL_IRQ_END + 1)
>
> This is spelled wrong (inteRnal) but where is that #define coming from
> anyway? And is this really used? I think this line can be simply deleted.

it comes from mach/irqs.h

>
>> +static inline struct sirfsoc_gpio_bank *sirfsoc_irq_to_bank(unsigned int irq)
>> +{
>> + ? ? ? return &sgpio_bank[(irq - SIRFSOC_GPIO_IRQ_START) / SIRFSOC_GPIO_BANK_SIZE];
>> +}
>> +
>> +static inline int sirfsoc_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
>> +{
>> + ? ? ? return SIRFSOC_GPIO_IRQ_START + (chip->base + offset);
>> +}
>
> Ick! Can you please use irqdomain for this?

agree.

>
> See the recent drivers/gpio/gpio-em.c driver for an excellent example.
>
>> +static int sirfsoc_gpio_irq_type(struct irq_data *d, unsigned type)
>> +{
>> + ? ? ? struct sirfsoc_gpio_bank *bank = sirfsoc_irq_to_bank(d->irq);
>> + ? ? ? int idx = sirfsoc_irq_to_offset(d->irq);
>> + ? ? ? u32 status, offset;
>
> "status" is a real bad name for this variable since you're not
> checking or changing any status by it. Use "val" or something.

agree.

>
>> + ? ? ? unsigned long flags;
>> +
>> + ? ? ? offset = SIRFSOC_GPIO_CTRL(bank->id, idx);
>> +
>> + ? ? ? spin_lock_irqsave(&sgpio_lock, flags);
>> +
>> + ? ? ? status = readl(bank->chip.regs + offset);
>> + ? ? ? status &= ~SIRFSOC_GPIO_CTL_INTR_STS_MASK;
>> +
>> + ? ? ? switch (type) {
>> + ? ? ? case IRQ_TYPE_NONE:
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? case IRQ_TYPE_EDGE_RISING:
>> + ? ? ? ? ? ? ? status |= (SIRFSOC_GPIO_CTL_INTR_HIGH_MASK | SIRFSOC_GPIO_CTL_INTR_TYPE_MASK);
>> + ? ? ? ? ? ? ? status &= ~SIRFSOC_GPIO_CTL_INTR_LOW_MASK;
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? case IRQ_TYPE_EDGE_FALLING:
>> + ? ? ? ? ? ? ? status &= ~SIRFSOC_GPIO_CTL_INTR_HIGH_MASK;
>> + ? ? ? ? ? ? ? status |= (SIRFSOC_GPIO_CTL_INTR_LOW_MASK | SIRFSOC_GPIO_CTL_INTR_TYPE_MASK);
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? case IRQ_TYPE_EDGE_BOTH:
>> + ? ? ? ? ? ? ? status |=
>> + ? ? ? ? ? ? ? ? ? ? ? (SIRFSOC_GPIO_CTL_INTR_HIGH_MASK | SIRFSOC_GPIO_CTL_INTR_LOW_MASK |
>> + ? ? ? ? ? ? ? ? ? ? ? ?SIRFSOC_GPIO_CTL_INTR_TYPE_MASK);
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? case IRQ_TYPE_LEVEL_LOW:
>> + ? ? ? ? ? ? ? status &= ~(SIRFSOC_GPIO_CTL_INTR_HIGH_MASK | SIRFSOC_GPIO_CTL_INTR_TYPE_MASK);
>> + ? ? ? ? ? ? ? status |= SIRFSOC_GPIO_CTL_INTR_LOW_MASK;
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? case IRQ_TYPE_LEVEL_HIGH:
>> + ? ? ? ? ? ? ? status |= SIRFSOC_GPIO_CTL_INTR_HIGH_MASK;
>> + ? ? ? ? ? ? ? status &= ~(SIRFSOC_GPIO_CTL_INTR_LOW_MASK | SIRFSOC_GPIO_CTL_INTR_TYPE_MASK);
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? }
>> +
>> + ? ? ? writel(status, bank->chip.regs + offset);
>> +
>> + ? ? ? spin_unlock_irqrestore(&sgpio_lock, flags);
>> +
>> + ? ? ? return 0;
>> +}
> (...)
>> + ? ? ? ? ? ? ? /*
>> + ? ? ? ? ? ? ? ?* Here we must check whether the corresponding GPIO's interrupt
>> + ? ? ? ? ? ? ? ?* has been enabled, otherwise just skip it
>> + ? ? ? ? ? ? ? ?*/
>> + ? ? ? ? ? ? ? if ((status & 0x1) && (ctrl & SIRFSOC_GPIO_CTL_INTR_EN_MASK)) {
>> + ? ? ? ? ? ? ? ? ? ? ? pr_debug("%s: gpio id %d idx %d happens\n",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? __func__, bank->id, idx);
>> + ? ? ? ? ? ? ? ? ? ? ? irq =
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? (SIRFSOC_GPIO_IRQ_START +
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?(bank->id * SIRFSOC_GPIO_BANK_SIZE)) + idx;
>
> So this is where irqdomain makes things simpler.

agree.

>
>> + ? ? ? ? ? ? ? ? ? ? ? generic_handle_irq(irq);
>> + ? ? ? ? ? ? ? }
>> +
>> + ? ? ? ? ? ? ? idx++;
>> + ? ? ? ? ? ? ? status = status >> 1;
>> + ? ? ? }
>> +}
>
>> +static inline void sirfsoc_gpio_set_output(struct sirfsoc_gpio_bank *bank, unsigned offset,
>> + ? ? ? int value)
>> +{
>> + ? ? ? u32 status;
>
> Again what does this has to do with any status...
>
>> +static void sirfsoc_gpio_set_value(struct gpio_chip *chip, unsigned offset,
>> + ? ? ? int value)
>> +{
>> + ? ? ? struct sirfsoc_gpio_bank *bank = sirfsoc_irqchip_to_bank(chip);
>> + ? ? ? u32 status;
>
> Neither this.
>
> Apart from that it looks good.
>
> Yours,
> Linus Walleij

Thanks
barry

[PATCH] PINCTRL: SiRF: add CSR SiRFprimaII GPIO driver depending on pinmux APIs

[Linus Walleij]

On Mon, May 28, 2012 at 11:03 PM, Barry Song <21cnbao@gmail.com> wrote:
> 2012/5/24 Linus Walleij <linus.walleij@linaro.org>:

>> This is actually such a nice and clean cut that it can be put into
>> drivers/gpio, it's just using pinctrl, not sharing code or hardware
>> registers with it. (If I understand correctly.)
>
> it is actually one pinmux/gpio/irq controller. as you see, i am using
> the same dt node with pinctrl-sirf.c:
> static const struct of_device_id sgpio_of_match[] __devinitconst = {
> ? ? ? {.compatible = "sirf,prima2-gpio-pinmux", },
> ? ? ? {},
> };
>
> here i splitted into two files just for viewing, and at the same time,
> i remap the same memory twice in pinctrl-sirf.c and gpio-sirf.c. they
> share same memory area.

Hmmmm.... but do they ever write into the same registers?

> these codes might be in pinctrl-sirf.c directly?

That also works. If you want one hardware block to be handled by
one driver you should do that. But if the split version works you
can also do that, so I'm not sure.

Yours,
Linus Walleij

[PATCH] PINCTRL: SiRF: add CSR SiRFprimaII GPIO driver depending on pinmux APIs

[Barry Song]

Hi Linus,

2012/5/29 Linus Walleij <linus.walleij@linaro.org>:
> On Mon, May 28, 2012 at 11:03 PM, Barry Song <21cnbao@gmail.com> wrote:
>> 2012/5/24 Linus Walleij <linus.walleij@linaro.org>:
>
>>> This is actually such a nice and clean cut that it can be put into
>>> drivers/gpio, it's just using pinctrl, not sharing code or hardware
>>> registers with it. (If I understand correctly.)
>>
>> it is actually one pinmux/gpio/irq controller. as you see, i am using
>> the same dt node with pinctrl-sirf.c:
>> static const struct of_device_id sgpio_of_match[] __devinitconst = {
>> ? ? ? {.compatible = "sirf,prima2-gpio-pinmux", },
>> ? ? ? {},
>> };
>>
>> here i splitted into two files just for viewing, and at the same time,
>> i remap the same memory twice in pinctrl-sirf.c and gpio-sirf.c. they
>> share same memory area.
>
> Hmmmm.... but do they ever write into the same registers?

they share the same IOMEM area, but gpio set registers have different
offset with pinmux. is there any difference between writing same
registers and writing different registers? mainly, they are from the
same hardware controller.

>
>> these codes might be in pinctrl-sirf.c directly?
>
> That also works. If you want one hardware block to be handled by
> one driver you should do that. But if the split version works you
> can also do that, so I'm not sure.
>
> Yours,
> Linus Walleij

Thanks
barry

[PATCH] PINCTRL: SiRF: add CSR SiRFprimaII GPIO driver depending on pinmux APIs

[Linus Walleij]

On Tue, May 29, 2012 at 8:20 AM, Barry Song <21cnbao@gmail.com> wrote:
> 2012/5/29 Linus Walleij <linus.walleij@linaro.org>:
>> On Mon, May 28, 2012 at 11:03 PM, Barry Song <21cnbao@gmail.com> wrote:
>>>
>>> here i splitted into two files just for viewing, and at the same time,
>>> i remap the same memory twice in pinctrl-sirf.c and gpio-sirf.c. they
>>> share same memory area.
>>
>> Hmmmm.... but do they ever write into the same registers?
>
> they share the same IOMEM area, but gpio set registers have different
> offset with pinmux. is there any difference between writing same
> registers and writing different registers? mainly, they are from the
> same hardware controller.

I don't really have an opinion on this, you can do it either way.

It's nice to split the files into separate drivers for smaller files and
cleanly cut devices.

It's also nice to have it all in one file so you can get the whole picture
of how the hardware works.

Usually the problem has been to cross-reference the GPIO ranges
from another driver, like adding the struct gpio_chip * to the ranges,
but it seems you don't have this problem in your driver? If you think
you will run into this problem, merge it into the pinctrl driver to
save yourself from lots of trouble.

Maybe someone else has some ideas?

Yours,
Linus Walleij

[PATCH] PINCTRL: SiRF: add CSR SiRFprimaII GPIO driver depending on pinmux APIs

[Barry Song]

Hi Linus,

2012/5/29 Linus Walleij <linus.walleij@linaro.org>:
> On Tue, May 29, 2012 at 8:20 AM, Barry Song <21cnbao@gmail.com> wrote:
>> 2012/5/29 Linus Walleij <linus.walleij@linaro.org>:
>>> On Mon, May 28, 2012 at 11:03 PM, Barry Song <21cnbao@gmail.com> wrote:
>>>>
>>>> here i splitted into two files just for viewing, and at the same time,
>>>> i remap the same memory twice in pinctrl-sirf.c and gpio-sirf.c. they
>>>> share same memory area.
>>>
>>> Hmmmm.... but do they ever write into the same registers?
>>
>> they share the same IOMEM area, but gpio set registers have different
>> offset with pinmux. is there any difference between writing same
>> registers and writing different registers? mainly, they are from the
>> same hardware controller.
>
> I don't really have an opinion on this, you can do it either way.
>
> It's nice to split the files into separate drivers for smaller files and
> cleanly cut devices.
>
> It's also nice to have it all in one file so you can get the whole picture
> of how the hardware works.

i think i'd better to have one driver for both. if i move to
gpio-generic driver for gpio module, gpio-generic driver will not
request gpio from pinctrl, but i need. and on the other hand,
gpio-generic doesn't include irq_chip, but i need.
so to me, gpio-generic driver is much like just for a single-function
gpio controller not good for a multi-function pinmux/gpio/irq
controller.

>
> Usually the problem has been to cross-reference the GPIO ranges
> from another driver, like adding the struct gpio_chip * to the ranges,
> but it seems you don't have this problem in your driver? If you think
> you will run into this problem, merge it into the pinctrl driver to
> save yourself from lots of trouble.

for the range issue, basically we have a (pinmux pin, gpio, irq) pair,
one pinmux pin can be a gpio pin and an external irq source , so
actually, i need to pinctrl_request_gpio() in my gpio_chip.request().
i would think prima2 pinctrl driver can be much like pinctrl-nomadik.c
and pinctrl-coh901.c, which are all of pinmux, gpio and irq chip.  i
might follow them and improve pinctrl-sirf.c to include gpio/irq too.

>
> Maybe someone else has some ideas?
>
> Yours,
> Linus Walleij

Thanks
barry

[PATCH] PINCTRL: SiRF: add CSR SiRFprimaII GPIO driver depending on pinmux APIs

[Linus Walleij]

On Sun, Jun 10, 2012 at 3:16 PM, Barry Song <21cnbao@gmail.com> wrote:
> 2012/5/29 Linus Walleij <linus.walleij@linaro.org>:

>> I don't really have an opinion on this, you can do it either way.
>>
>> It's nice to split the files into separate drivers for smaller files and
>> cleanly cut devices.
>>
>> It's also nice to have it all in one file so you can get the whole picture
>> of how the hardware works.
>
> i think i'd better to have one driver for both. if i move to
> gpio-generic driver for gpio module, gpio-generic driver will not
> request gpio from pinctrl, but i need. and on the other hand,
> gpio-generic doesn't include irq_chip, but i need.
> so to me, gpio-generic driver is much like just for a single-function
> gpio controller not good for a multi-function pinmux/gpio/irq
> controller.

OK that makes sense I thought you didn't need to cross-correlate
them.

>> Usually the problem has been to cross-reference the GPIO ranges
>> from another driver, like adding the struct gpio_chip * to the ranges,
>> but it seems you don't have this problem in your driver? If you think
>> you will run into this problem, merge it into the pinctrl driver to
>> save yourself from lots of trouble.
>
> for the range issue, basically we have a (pinmux pin, gpio, irq) pair,
> one pinmux pin can be a gpio pin and an external irq source , so
> actually, i need to pinctrl_request_gpio() in my gpio_chip.request().
> i would think prima2 pinctrl driver can be much like pinctrl-nomadik.c
> and pinctrl-coh901.c, which are all of pinmux, gpio and irq chip. ?i
> might follow them and improve pinctrl-sirf.c to include gpio/irq too.

Yep, but now I see you haven't used irqdomain to map between
gpios and IRQs so I will hit you with one more review comment.
We try to get all gpio offset -> IRQ mapping done using
irqdomains.

Yours,
Linus Walleij

[PATCH] PINCTRL: SiRF: add CSR SiRFprimaII GPIO driver depending on pinmux APIs

[Linus Walleij]

On Tue, May 22, 2012 at 9:14 AM, Barry Song <Barry.Song@csr.com> wrote:

> +static inline struct sirfsoc_gpio_bank *sirfsoc_irq_to_bank(unsigned int irq)
> +{
> + ? ? ? return &sgpio_bank[(irq - SIRFSOC_GPIO_IRQ_START) / SIRFSOC_GPIO_BANK_SIZE];
> +}
> +
> +static inline int sirfsoc_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
> +{
> + ? ? ? return SIRFSOC_GPIO_IRQ_START + (chip->base + offset);
> +}
> +
> +static inline int sirfsoc_irq_to_offset(unsigned int irq)
> +{
> + ? ? ? return (irq - SIRFSOC_GPIO_IRQ_START) % SIRFSOC_GPIO_BANK_SIZE;
> +}
> +
> +static inline int sirfsoc_gpio_to_offset(unsigned int gpio)
> +{
> + ? ? ? return gpio % SIRFSOC_GPIO_BANK_SIZE;
> +}

Some of this is doing the work of irqdomain, which is supposed to map a certain
local IRQ number to the global space (I guess that is what is denoted by
SIRFSOC_GPIO_IRQ_START.)

Can you please look into using irqdomain for this?

There are several examples in drivers/gpio/* and drivers/pinctrl/*
for example:

drivers/gpio/gpio-pxa.c
drivers/pinctrl/pinctrl-nomadik.c


Yours,
Linus Walleij

[PATCH] PINCTRL: SiRF: add CSR SiRFprimaII GPIO driver depending on pinmux APIs

[Barry Song]

2012/6/12 Linus Walleij <linus.walleij@linaro.org>
>
> On Tue, May 22, 2012 at 9:14 AM, Barry Song <Barry.Song@csr.com> wrote:
>
> > +static inline struct sirfsoc_gpio_bank *sirfsoc_irq_to_bank(unsigned
> > int irq)
> > +{
> > + ? ? ? return &sgpio_bank[(irq - SIRFSOC_GPIO_IRQ_START) /
> > SIRFSOC_GPIO_BANK_SIZE];
> > +}
> > +
> > +static inline int sirfsoc_gpio_to_irq(struct gpio_chip *chip, unsigned
> > offset)
> > +{
> > + ? ? ? return SIRFSOC_GPIO_IRQ_START + (chip->base + offset);
> > +}
> > +
> > +static inline int sirfsoc_irq_to_offset(unsigned int irq)
> > +{
> > + ? ? ? return (irq - SIRFSOC_GPIO_IRQ_START) % SIRFSOC_GPIO_BANK_SIZE;
> > +}
> > +
> > +static inline int sirfsoc_gpio_to_offset(unsigned int gpio)
> > +{
> > + ? ? ? return gpio % SIRFSOC_GPIO_BANK_SIZE;
> > +}
>
> Some of this is doing the work of irqdomain, which is supposed to map a
> certain
> local IRQ number to the global space (I guess that is what is denoted by
> SIRFSOC_GPIO_IRQ_START.)
>
> Can you please look into using irqdomain for this?

yes. sure. Thanks!

>
> There are several examples in drivers/gpio/* and drivers/pinctrl/*
> for example:
>
> drivers/gpio/gpio-pxa.c
> drivers/pinctrl/pinctrl-nomadik.c
>
>
> Yours,
> Linus Walleij

-barry