Skip to contentSkip to conversation

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

PATCH New Barry Song 4 participants 11 messages May 22, 2012 – Jun 12, 2012

Subsystems (4)
Device tree bindings Arm (32-bit) Arm sub-architectures Pin control
lore
Share & tools
lore

Barry Song

author
#0 lore raw

PATCH 1/1 PINCTRL: SiRF: add CSR SiRFprimaII GPIO driver depending on pinmux APIs

From: Barry Song <[email protected]>

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 <[email protected]>
Signed-off-by Barry Song <[email protected]>
---
 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
arch/arm/boot/dts/prima2-cb.dts +1 -0
index 34ae3a6..4245306 100644
@@ -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;
 			};
arch/arm/mach-prima2/include/mach/gpio.h new file +1 -0
new file mode 100644
index 0000000..40a8c17
@@ -0,0 +1 @@
+/* empty */
drivers/pinctrl/Makefile +1 -1
index 515e32f..90b699b 100644
@@ -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
drivers/pinctrl/gpio-sirf.c new file +458 -0

458 lines not on this page · raw message

Signature · 6 lines
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
#1 lore raw

PATCH 1/1 PINCTRL: SiRF: add CSR SiRFprimaII GPIO driver depending on pinmux APIs

Quoted 6 lines · Barry Song

From: Barry Song <[email protected]>

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).

Quoted 6 lines

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 <[email protected]>
Signed-off-by Barry Song <[email protected]>

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.

Quoted 1 line

+#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.

drivers/pinctrl/gpio-sirf.c line 63

Quoted 9 lines
drivers/pinctrl/gpio-sirf.c new file +9 -0
+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.

Review

Quoted 5 lines
(quoted diff) +5 -0
+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.

Quoted 41 lines

+ ? ? ? unsigned long flags;

(quoted diff) +40 -0
+
+ ? ? ? 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;
+}

(...)

Quoted 10 lines

+ ? ? ? ? ? ? ? /*
+ ? ? ? ? ? ? ? ?* 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.

Review

Quoted 12 lines

+ ? ? ? ? ? ? ? ? ? ? ? generic_handle_irq(irq);
+ ? ? ? ? ? ? ? }

(quoted diff) +9 -0
+
+ ? ? ? ? ? ? ? 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...

Review

Quoted 5 lines
(quoted diff) +5 -0
+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

#2 lore raw

PATCH 1/1 PINCTRL: SiRF: add CSR SiRFprimaII GPIO driver depending on pinmux APIs

Quoted 12 lines · Linus Walleij
Quoted 10 lines · Barry Song

From: Barry Song <[email protected]>

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.

Quoted 8 lines
Quoted 2 lines
Signed-off-by Yuping Luo <[email protected]>
Signed-off-by Barry Song <[email protected]>

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

Quoted 6 lines
Quoted 2 lines

+#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.

Quoted 11 lines

drivers/pinctrl/gpio-sirf.c line 63

Quoted 9 lines
drivers/pinctrl/gpio-sirf.c new file +9 -0
+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.

Quoted 91 lines

Review

Quoted 5 lines
(quoted diff) +5 -0
+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.

Quoted 41 lines

+ ?? ?? ?? unsigned long flags;

(quoted diff) +40 -0
+
+ ?? ?? ?? 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;
+}

(...)

Quoted 10 lines

+ ?? ?? ?? ?? ?? ?? ?? /*
+ ?? ?? ?? ?? ?? ?? ?? ??* 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.

Review

Quoted 12 lines

+ ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? generic_handle_irq(irq);
+ ?? ?? ?? ?? ?? ?? ?? }

(quoted diff) +9 -0
+
+ ?? ?? ?? ?? ?? ?? ?? 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...

Review

Quoted 5 lines
(quoted diff) +5 -0
+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

Signature · 2 lines
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.
#3 lore raw

PATCH 1/1 PINCTRL: SiRF: add CSR SiRFprimaII GPIO driver depending on pinmux APIs

In reply to Linus Walleij

Hi Linus,
Thanks!

2012/5/24 Linus Walleij <[email protected]>:

Quoted 12 lines · Barry Song
Quoted 11 lines

From: Barry Song <[email protected]>

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.

Quoted 6 lines
Quoted 2 lines
Signed-off-by Yuping Luo <[email protected]>
Signed-off-by Barry Song <[email protected]>

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?

Quoted 7 lines

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

Quoted 1 line

+#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

Quoted 12 lines

drivers/pinctrl/gpio-sirf.c line 63

Quoted 10 lines
drivers/pinctrl/gpio-sirf.c new file +9 -0
+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.

Quoted 11 lines

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

Review

Quoted 5 lines
(quoted diff) +5 -0
+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.

Quoted 54 lines
Quoted 42 lines

+ ? ? ? unsigned long flags;

(quoted diff) +40 -0
+
+ ? ? ? 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;
+}

(...)

Quoted 10 lines

+ ? ? ? ? ? ? ? /*
+ ? ? ? ? ? ? ? ?* 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;

Quoted 1 line

So this is where irqdomain makes things simpler.

agree.

Quoted 24 lines

Review

Quoted 13 lines

+ ? ? ? ? ? ? ? ? ? ? ? generic_handle_irq(irq);
+ ? ? ? ? ? ? ? }

(quoted diff) +9 -0
+
+ ? ? ? ? ? ? ? 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...

Review

Quoted 5 lines
(quoted diff) +5 -0
+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.

Quoted 4 lines

Apart from that it looks good.

Yours,
Linus Walleij

Thanks
barry

#4 lore raw

PATCH 1/1 PINCTRL: SiRF: add CSR SiRFprimaII GPIO driver depending on pinmux APIs

Quoted 13 lines · Barry Song

2012/5/24 Linus Walleij <[email protected]>:

Quoted 3 lines

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", },
? ? ? {},
};

Quoted 3 lines

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?

Quoted 1 line

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

#5 lore raw

PATCH 1/1 PINCTRL: SiRF: add CSR SiRFprimaII GPIO driver depending on pinmux APIs

Hi Linus,

2012/5/29 Linus Walleij <[email protected]>:

Quoted 14 lines · Barry Song
Quoted 13 lines

2012/5/24 Linus Walleij <[email protected]>:

Quoted 3 lines

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", },
? ? ? {},
};

Quoted 5 lines
Quoted 3 lines

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.

Quoted 9 lines
Quoted 2 lines

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

#6 lore raw

PATCH 1/1 PINCTRL: SiRF: add CSR SiRFprimaII GPIO driver depending on pinmux APIs

Quoted 7 lines · Barry Song

2012/5/29 Linus Walleij <[email protected]>:

Quoted 6 lines · Barry Song
Quoted 4 lines

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.

Quoted 6 lines
Quoted 1 line

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

#7 lore raw

PATCH 1/1 PINCTRL: SiRF: add CSR SiRFprimaII GPIO driver depending on pinmux APIs

Hi Linus,

2012/5/29 Linus Walleij <[email protected]>:

Quoted 15 lines · Barry Song
Quoted 7 lines

2012/5/29 Linus Walleij <[email protected]>:

Quoted 6 lines · Barry Song
Quoted 4 lines

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.

Quoted 6 lines
Quoted 1 line

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.

Quoted 7 lines

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.

Quoted 6 lines

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.

Quoted 5 lines

Maybe someone else has some ideas?

Yours,
Linus Walleij

Thanks
barry

#8 lore raw

PATCH 1/1 PINCTRL: SiRF: add CSR SiRFprimaII GPIO driver depending on pinmux APIs

Quoted 10 lines · Barry Song

2012/5/29 Linus Walleij <[email protected]>:

Quoted 7 lines

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.

Quoted 7 lines

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.

Quoted 6 lines
Quoted 5 lines

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.

Quoted 6 lines

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

#9 lore raw

PATCH 1/1 PINCTRL: SiRF: add CSR SiRFprimaII GPIO driver depending on pinmux APIs

In reply to Barry Song
Quoted 9 lines · Barry Song
drivers/pinctrl/gpio-sirf.c new file +9 -0
+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);
+}

drivers/pinctrl/gpio-sirf.c line 73

Quoted 10 lines
drivers/pinctrl/gpio-sirf.c new file +10 -0
+
+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

#10 lore raw

PATCH 1/1 PINCTRL: SiRF: add CSR SiRFprimaII GPIO driver depending on pinmux APIs

2012/6/12 Linus Walleij <[email protected]>

Quoted 31 lines · Barry Song
Quoted 13 lines

+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];

drivers/pinctrl/gpio-sirf.c new file +3 -0
+}
+
+static inline int sirfsoc_gpio_to_irq(struct gpio_chip *chip, unsigned

offset)

drivers/pinctrl/gpio-sirf.c new file +3 -0
+{
+ ? ? ? return SIRFSOC_GPIO_IRQ_START + (chip->base + offset);
+}

drivers/pinctrl/gpio-sirf.c line 73

Quoted 10 lines
drivers/pinctrl/gpio-sirf.c new file +10 -0
+
+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.)

Quoted 1 line

Can you please look into using irqdomain for this?

yes. sure. Thanks!

Quoted 10 lines

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