[PATCH 0/4] gpio: pxa: integrate with pincontrol

[PATCH 0/4] gpio: pxa: integrate with pincontrol

[Robert Jarzmik]

Hi Linus, Alexandre and Haojian,

This serie aims at several cleanups and improvements in the pxa gpio driver, to
support integration with pin control.

Amongst the changes, the main points are :
 - removal of as many global variables as possible
 - use of devm_*() function when applicable
 - use of irqdomain
 - use of pinctrl functions when available

Haojian, I've tested that on pxa27x, in both platform_data and devicetree
builds. Nevertheless I'd feel better if others architectures were tested,
especially mmp ones and pxa1928, which I don't have home.

Happy review.

Robert Jarzmik (4):
  gpio: pxa: convert to one gpiochip
  gpio: pxa: convert to devm_ioremap
  gpio: pxa: change the interrupt management
  gpio: pxa: add pin control gpio direction and request

 drivers/gpio/gpio-pxa.c | 370 ++++++++++++++++++++++++++++--------------------
 1 file changed, 214 insertions(+), 156 deletions(-)

-- 
2.1.4

[PATCH 1/4] gpio: pxa: convert to one gpiochip

[Robert Jarzmik]

The pxa gpio IP is provided by one chip, which holds multiple banks.

Another reason the driver should register only one gpiochip instead of
multiple gpiochips (ie. 1 per each bank) is that for pincontrol and
devicetree integration (think gpio-ranges), it's impossible to have the
contiguous pin range 0..127 mapped to gpios 0..127.

This patch, amongst other thinks, paves the path to loosen the bond with
the global structure variable pxa_gpio_chip.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/gpio/gpio-pxa.c | 221 ++++++++++++++++++++++++++----------------------
 1 file changed, 120 insertions(+), 101 deletions(-)

diff --git a/drivers/gpio/gpio-pxa.c b/drivers/gpio/gpio-pxa.c
index df2ce550f309..de2cfbeccaa1 100644
--- a/drivers/gpio/gpio-pxa.c
+++ b/drivers/gpio/gpio-pxa.c
@@ -69,15 +69,11 @@ static struct irq_domain *domain;
 static struct device_node *pxa_gpio_of_node;
 #endif
 
-struct pxa_gpio_chip {
-	struct gpio_chip chip;
+struct pxa_gpio_bank {
 	void __iomem	*regbase;
-	char label[10];
-
 	unsigned long	irq_mask;
 	unsigned long	irq_edge_rise;
 	unsigned long	irq_edge_fall;
-	int (*set_wake)(unsigned int gpio, unsigned int on);
 
 #ifdef CONFIG_PM
 	unsigned long	saved_gplr;
@@ -87,6 +83,16 @@ struct pxa_gpio_chip {
 #endif
 };
 
+struct pxa_gpio_chip {
+	struct device *dev;
+	struct gpio_chip chip;
+	struct pxa_gpio_bank *banks;
+
+	int irq0;
+	int irq1;
+	int (*set_wake)(unsigned int gpio, unsigned int on);
+};
+
 enum pxa_gpio_type {
 	PXA25X_GPIO = 0,
 	PXA26X_GPIO,
@@ -104,9 +110,8 @@ struct pxa_gpio_id {
 };
 
 static DEFINE_SPINLOCK(gpio_lock);
-static struct pxa_gpio_chip *pxa_gpio_chips;
+static struct pxa_gpio_chip *pxa_gpio_chip;
 static enum pxa_gpio_type gpio_type;
-static void __iomem *gpio_reg_base;
 
 static struct pxa_gpio_id pxa25x_id = {
 	.type		= PXA25X_GPIO,
@@ -148,17 +153,27 @@ static struct pxa_gpio_id pxa1928_id = {
 	.gpio_nums	= 224,
 };
 
-#define for_each_gpio_chip(i, c)			\
-	for (i = 0, c = &pxa_gpio_chips[0]; i <= pxa_last_gpio; i += 32, c++)
+#define for_each_gpio_bank(i, b, pc)					\
+	for (i = 0, b = pc->banks; i <= pxa_last_gpio; i += 32, b++)
 
-static inline void __iomem *gpio_chip_base(struct gpio_chip *c)
+static inline struct pxa_gpio_chip *chip_to_pxachip(struct gpio_chip *c)
 {
-	return container_of(c, struct pxa_gpio_chip, chip)->regbase;
+	struct pxa_gpio_chip *pxa_chip =
+		container_of(c, struct pxa_gpio_chip, chip);
+
+	return pxa_chip;
 }
+static inline void __iomem *gpio_bank_base(struct gpio_chip *c, int gpio)
+{
+	struct pxa_gpio_bank *bank = chip_to_pxachip(c)->banks + (gpio / 32);
 
-static inline struct pxa_gpio_chip *gpio_to_pxachip(unsigned gpio)
+	return bank->regbase;
+}
+
+static inline struct pxa_gpio_bank *gpio_to_pxabank(struct gpio_chip *c,
+						    unsigned gpio)
 {
-	return &pxa_gpio_chips[gpio_to_bank(gpio)];
+	return chip_to_pxachip(c)->banks + gpio / 32;
 }
 
 static inline int gpio_is_pxa_type(int type)
@@ -187,15 +202,13 @@ static inline int __gpio_is_inverted(int gpio)
  * is attributed as "occupied" here (I know this terminology isn't
  * accurate, you are welcome to propose a better one :-)
  */
-static inline int __gpio_is_occupied(unsigned gpio)
+static inline int __gpio_is_occupied(struct pxa_gpio_chip *pchip, unsigned gpio)
 {
-	struct pxa_gpio_chip *pxachip;
 	void __iomem *base;
 	unsigned long gafr = 0, gpdr = 0;
 	int ret, af = 0, dir = 0;
 
-	pxachip = gpio_to_pxachip(gpio);
-	base = gpio_chip_base(&pxachip->chip);
+	base = gpio_bank_base(&pchip->chip, gpio);
 	gpdr = readl_relaxed(base + GPDR_OFFSET);
 
 	switch (gpio_type) {
@@ -220,7 +233,7 @@ static inline int __gpio_is_occupied(unsigned gpio)
 
 static int pxa_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
 {
-	return chip->base + offset + irq_base;
+	return offset + irq_base;
 }
 
 int pxa_irq_to_gpio(int irq)
@@ -230,8 +243,8 @@ int pxa_irq_to_gpio(int irq)
 
 static int pxa_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
 {
-	void __iomem *base = gpio_chip_base(chip);
-	uint32_t value, mask = 1 << offset;
+	void __iomem *base = gpio_bank_base(chip, offset);
+	uint32_t value, mask = GPIO_bit(offset);
 	unsigned long flags;
 
 	spin_lock_irqsave(&gpio_lock, flags);
@@ -250,8 +263,8 @@ static int pxa_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
 static int pxa_gpio_direction_output(struct gpio_chip *chip,
 				     unsigned offset, int value)
 {
-	void __iomem *base = gpio_chip_base(chip);
-	uint32_t tmp, mask = 1 << offset;
+	void __iomem *base = gpio_bank_base(chip, offset);
+	uint32_t tmp, mask = GPIO_bit(offset);
 	unsigned long flags;
 
 	writel_relaxed(mask, base + (value ? GPSR_OFFSET : GPCR_OFFSET));
@@ -271,14 +284,18 @@ static int pxa_gpio_direction_output(struct gpio_chip *chip,
 
 static int pxa_gpio_get(struct gpio_chip *chip, unsigned offset)
 {
-	u32 gplr = readl_relaxed(gpio_chip_base(chip) + GPLR_OFFSET);
-	return !!(gplr & (1 << offset));
+	void __iomem *base = gpio_bank_base(chip, offset);
+	u32 gplr = readl_relaxed(base + GPLR_OFFSET);
+
+	return !!(gplr & GPIO_bit(offset));
 }
 
 static void pxa_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 {
-	writel_relaxed(1 << offset, gpio_chip_base(chip) +
-				(value ? GPSR_OFFSET : GPCR_OFFSET));
+	void __iomem *base = gpio_bank_base(chip, offset);
+
+	writel_relaxed(GPIO_bit(offset),
+		       base + (value ? GPSR_OFFSET : GPCR_OFFSET));
 }
 
 #ifdef CONFIG_OF_GPIO
@@ -289,61 +306,49 @@ static int pxa_gpio_of_xlate(struct gpio_chip *gc,
 	if (gpiospec->args[0] > pxa_last_gpio)
 		return -EINVAL;
 
-	if (gc != &pxa_gpio_chips[gpiospec->args[0] / 32].chip)
-		return -EINVAL;
-
 	if (flags)
 		*flags = gpiospec->args[1];
 
-	return gpiospec->args[0] % 32;
+	return gpiospec->args[0];
 }
 #endif
 
-static int pxa_init_gpio_chip(int gpio_end,
-					int (*set_wake)(unsigned int, unsigned int))
+static int pxa_init_gpio_chip(struct pxa_gpio_chip *pchip, int ngpio,
+			      void __iomem *regbase)
 {
-	int i, gpio, nbanks = gpio_to_bank(gpio_end) + 1;
-	struct pxa_gpio_chip *chips;
+	int i, gpio, nbanks = DIV_ROUND_UP(ngpio, 32);
+	struct pxa_gpio_bank *bank;
 
-	chips = kzalloc(nbanks * sizeof(struct pxa_gpio_chip), GFP_KERNEL);
-	if (chips == NULL) {
-		pr_err("%s: failed to allocate GPIO chips\n", __func__);
+	pchip->banks = devm_kcalloc(pchip->dev, nbanks, sizeof(*pchip->banks),
+				    GFP_KERNEL);
+	if (!pchip->banks)
 		return -ENOMEM;
-	}
 
-	for (i = 0, gpio = 0; i < nbanks; i++, gpio += 32) {
-		struct gpio_chip *c = &chips[i].chip;
-
-		sprintf(chips[i].label, "gpio-%d", i);
-		chips[i].regbase = gpio_reg_base + BANK_OFF(i);
-		chips[i].set_wake = set_wake;
-
-		c->base  = gpio;
-		c->label = chips[i].label;
-
-		c->direction_input  = pxa_gpio_direction_input;
-		c->direction_output = pxa_gpio_direction_output;
-		c->get = pxa_gpio_get;
-		c->set = pxa_gpio_set;
-		c->to_irq = pxa_gpio_to_irq;
+	pchip->chip.label = "gpio-pxa";
+	pchip->chip.direction_input  = pxa_gpio_direction_input;
+	pchip->chip.direction_output = pxa_gpio_direction_output;
+	pchip->chip.get = pxa_gpio_get;
+	pchip->chip.set = pxa_gpio_set;
+	pchip->chip.to_irq = pxa_gpio_to_irq;
+	pchip->chip.ngpio = ngpio;
 #ifdef CONFIG_OF_GPIO
-		c->of_node = pxa_gpio_of_node;
-		c->of_xlate = pxa_gpio_of_xlate;
-		c->of_gpio_n_cells = 2;
+	pchip->chip.of_node = pxa_gpio_of_node;
+	pchip->chip.of_xlate = pxa_gpio_of_xlate;
+	pchip->chip.of_gpio_n_cells = 2;
 #endif
 
-		/* number of GPIOs on last bank may be less than 32 */
-		c->ngpio = (gpio + 31 > gpio_end) ? (gpio_end - gpio + 1) : 32;
-		gpiochip_add(c);
+	for (i = 0, gpio = 0; i < nbanks; i++, gpio += 32) {
+		bank = pchip->banks + i;
+		bank->regbase = regbase + BANK_OFF(i);
 	}
-	pxa_gpio_chips = chips;
-	return 0;
+
+	return gpiochip_add(&pchip->chip);
 }
 
 /* Update only those GRERx and GFERx edge detection register bits if those
  * bits are set in c->irq_mask
  */
-static inline void update_edge_detect(struct pxa_gpio_chip *c)
+static inline void update_edge_detect(struct pxa_gpio_bank *c)
 {
 	uint32_t grer, gfer;
 
@@ -357,12 +362,11 @@ static inline void update_edge_detect(struct pxa_gpio_chip *c)
 
 static int pxa_gpio_irq_type(struct irq_data *d, unsigned int type)
 {
-	struct pxa_gpio_chip *c;
+	struct pxa_gpio_chip *pchip = pxa_gpio_chip;
 	int gpio = pxa_irq_to_gpio(d->irq);
+	struct pxa_gpio_bank *c = gpio_to_pxabank(&pchip->chip, gpio);
 	unsigned long gpdr, mask = GPIO_bit(gpio);
 
-	c = gpio_to_pxachip(gpio);
-
 	if (type == IRQ_TYPE_PROBE) {
 		/* Don't mess with enabled GPIOs using preconfigured edges or
 		 * GPIOs set to alternate function or to output during probe
@@ -370,7 +374,7 @@ static int pxa_gpio_irq_type(struct irq_data *d, unsigned int type)
 		if ((c->irq_edge_rise | c->irq_edge_fall) & GPIO_bit(gpio))
 			return 0;
 
-		if (__gpio_is_occupied(gpio))
+		if (__gpio_is_occupied(pchip, gpio))
 			return 0;
 
 		type = IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING;
@@ -403,18 +407,17 @@ static int pxa_gpio_irq_type(struct irq_data *d, unsigned int type)
 
 static void pxa_gpio_demux_handler(struct irq_desc *desc)
 {
-	struct pxa_gpio_chip *c;
-	int loop, gpio, gpio_base, n;
+	int loop, gpio, n, handled = 0;
 	unsigned long gedr;
 	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct pxa_gpio_chip *pchip = pxa_gpio_chip;
+	struct pxa_gpio_bank *c;
 
 	chained_irq_enter(chip, desc);
 
 	do {
 		loop = 0;
-		for_each_gpio_chip(gpio, c) {
-			gpio_base = c->chip.base;
-
+		for_each_gpio_bank(gpio, c, pchip) {
 			gedr = readl_relaxed(c->regbase + GEDR_OFFSET);
 			gedr = gedr & c->irq_mask;
 			writel_relaxed(gedr, c->regbase + GEDR_OFFSET);
@@ -422,7 +425,7 @@ static void pxa_gpio_demux_handler(struct irq_desc *desc)
 			for_each_set_bit(n, &gedr, BITS_PER_LONG) {
 				loop = 1;
 
-				generic_handle_irq(gpio_to_irq(gpio_base + n));
+				generic_handle_irq(gpio_to_irq(gpio + n));
 			}
 		}
 	} while (loop);
@@ -432,41 +435,45 @@ static void pxa_gpio_demux_handler(struct irq_desc *desc)
 
 static void pxa_ack_muxed_gpio(struct irq_data *d)
 {
+	struct pxa_gpio_chip *pchip = pxa_gpio_chip;
 	int gpio = pxa_irq_to_gpio(d->irq);
-	struct pxa_gpio_chip *c = gpio_to_pxachip(gpio);
+	void __iomem *base = gpio_bank_base(&pchip->chip, gpio);
 
-	writel_relaxed(GPIO_bit(gpio), c->regbase + GEDR_OFFSET);
+	writel_relaxed(GPIO_bit(gpio), base + GEDR_OFFSET);
 }
 
 static void pxa_mask_muxed_gpio(struct irq_data *d)
 {
+	struct pxa_gpio_chip *pchip = pxa_gpio_chip;
 	int gpio = pxa_irq_to_gpio(d->irq);
-	struct pxa_gpio_chip *c = gpio_to_pxachip(gpio);
+	struct pxa_gpio_bank *b = gpio_to_pxabank(&pchip->chip, gpio);
+	void __iomem *base = gpio_bank_base(&pchip->chip, gpio);
 	uint32_t grer, gfer;
 
-	c->irq_mask &= ~GPIO_bit(gpio);
+	b->irq_mask &= ~GPIO_bit(gpio);
 
-	grer = readl_relaxed(c->regbase + GRER_OFFSET) & ~GPIO_bit(gpio);
-	gfer = readl_relaxed(c->regbase + GFER_OFFSET) & ~GPIO_bit(gpio);
-	writel_relaxed(grer, c->regbase + GRER_OFFSET);
-	writel_relaxed(gfer, c->regbase + GFER_OFFSET);
+	grer = readl_relaxed(base + GRER_OFFSET) & ~GPIO_bit(gpio);
+	gfer = readl_relaxed(base + GFER_OFFSET) & ~GPIO_bit(gpio);
+	writel_relaxed(grer, base + GRER_OFFSET);
+	writel_relaxed(gfer, base + GFER_OFFSET);
 }
 
 static int pxa_gpio_set_wake(struct irq_data *d, unsigned int on)
 {
 	int gpio = pxa_irq_to_gpio(d->irq);
-	struct pxa_gpio_chip *c = gpio_to_pxachip(gpio);
+	struct pxa_gpio_chip *pchip = pxa_gpio_chip;
 
-	if (c->set_wake)
-		return c->set_wake(gpio, on);
+	if (pchip->set_wake)
+		return pchip->set_wake(gpio, on);
 	else
 		return 0;
 }
 
 static void pxa_unmask_muxed_gpio(struct irq_data *d)
 {
+	struct pxa_gpio_chip *pchip = pxa_gpio_chip;
 	int gpio = pxa_irq_to_gpio(d->irq);
-	struct pxa_gpio_chip *c = gpio_to_pxachip(gpio);
+	struct pxa_gpio_bank *c = gpio_to_pxabank(&pchip->chip, gpio);
 
 	c->irq_mask |= GPIO_bit(gpio);
 	update_edge_detect(c);
@@ -533,9 +540,10 @@ const struct irq_domain_ops pxa_irq_domain_ops = {
 	.xlate	= irq_domain_xlate_twocell,
 };
 
-static int pxa_gpio_probe_dt(struct platform_device *pdev)
+static int pxa_gpio_probe_dt(struct platform_device *pdev,
+			     struct pxa_gpio_chip *pchip)
 {
-	int ret = 0, nr_gpios;
+	int nr_gpios;
 	struct device_node *np = pdev->dev.of_node;
 	const struct of_device_id *of_id =
 				of_match_device(pxa_gpio_dt_ids, &pdev->dev);
@@ -554,40 +562,44 @@ static int pxa_gpio_probe_dt(struct platform_device *pdev)
 	irq_base = irq_alloc_descs(-1, 0, nr_gpios, 0);
 	if (irq_base < 0) {
 		dev_err(&pdev->dev, "Failed to allocate IRQ numbers\n");
-		ret = irq_base;
-		goto err;
+		return irq_base;
 	}
 	domain = irq_domain_add_legacy(np, nr_gpios, irq_base, 0,
-				       &pxa_irq_domain_ops, NULL);
+				       &pxa_irq_domain_ops, pchip);
 	pxa_gpio_of_node = np;
 	return 0;
-err:
-	iounmap(gpio_reg_base);
-	return ret;
 }
 #else
-#define pxa_gpio_probe_dt(pdev)		(-1)
+#define pxa_gpio_probe_dt(pdev, pchip)		(-1)
 #endif
 
 static int pxa_gpio_probe(struct platform_device *pdev)
 {
-	struct pxa_gpio_chip *c;
+	struct pxa_gpio_chip *pchip;
+	struct pxa_gpio_bank *c;
 	struct resource *res;
 	struct clk *clk;
 	struct pxa_gpio_platform_data *info;
+	void __iomem *gpio_reg_base;
 	int gpio, irq, ret, use_of = 0;
 	int irq0 = 0, irq1 = 0, irq_mux, gpio_offset = 0;
 
+	pchip = devm_kzalloc(&pdev->dev, sizeof(*pchip), GFP_KERNEL);
+	if (!pchip)
+		return -ENOMEM;
+	pchip->dev = &pdev->dev;
+
 	info = dev_get_platdata(&pdev->dev);
 	if (info) {
 		irq_base = info->irq_base;
 		if (irq_base <= 0)
 			return -EINVAL;
 		pxa_last_gpio = pxa_gpio_nums(pdev);
+		pchip->set_wake = info->gpio_set_wake;
 	} else {
 		irq_base = 0;
 		use_of = 1;
-		ret = pxa_gpio_probe_dt(pdev);
+		ret = pxa_gpio_probe_dt(pdev, pchip);
 		if (ret < 0)
 			return -EINVAL;
 	}
@@ -626,10 +638,14 @@ static int pxa_gpio_probe(struct platform_device *pdev)
 	}
 
 	/* Initialize GPIO chips */
-	pxa_init_gpio_chip(pxa_last_gpio, info ? info->gpio_set_wake : NULL);
+	ret = pxa_init_gpio_chip(pchip, pxa_last_gpio + 1, gpio_reg_base);
+	if (ret) {
+		clk_put(clk);
+		return ret;
+	}
 
 	/* clear all GPIO edge detects */
-	for_each_gpio_chip(gpio, c) {
+	for_each_gpio_bank(gpio, c, pchip) {
 		writel_relaxed(0, c->regbase + GFER_OFFSET);
 		writel_relaxed(0, c->regbase + GRER_OFFSET);
 		writel_relaxed(~0, c->regbase + GEDR_OFFSET);
@@ -664,6 +680,7 @@ static int pxa_gpio_probe(struct platform_device *pdev)
 		irq_set_chained_handler(irq0, pxa_gpio_demux_handler);
 	if (irq1 > 0)
 		irq_set_chained_handler(irq1, pxa_gpio_demux_handler);
+	pxa_gpio_chip = pchip;
 
 	irq_set_chained_handler(irq_mux, pxa_gpio_demux_handler);
 	return 0;
@@ -699,10 +716,11 @@ postcore_initcall(pxa_gpio_init);
 #ifdef CONFIG_PM
 static int pxa_gpio_suspend(void)
 {
-	struct pxa_gpio_chip *c;
+	struct pxa_gpio_chip *pchip = pxa_gpio_chip;
+	struct pxa_gpio_bank *c;
 	int gpio;
 
-	for_each_gpio_chip(gpio, c) {
+	for_each_gpio_bank(gpio, c, pchip) {
 		c->saved_gplr = readl_relaxed(c->regbase + GPLR_OFFSET);
 		c->saved_gpdr = readl_relaxed(c->regbase + GPDR_OFFSET);
 		c->saved_grer = readl_relaxed(c->regbase + GRER_OFFSET);
@@ -716,10 +734,11 @@ static int pxa_gpio_suspend(void)
 
 static void pxa_gpio_resume(void)
 {
-	struct pxa_gpio_chip *c;
+	struct pxa_gpio_chip *pchip = pxa_gpio_chip;
+	struct pxa_gpio_bank *c;
 	int gpio;
 
-	for_each_gpio_chip(gpio, c) {
+	for_each_gpio_bank(gpio, c, pchip) {
 		/* restore level with set/clear */
 		writel_relaxed(c->saved_gplr, c->regbase + GPSR_OFFSET);
 		writel_relaxed(~c->saved_gplr, c->regbase + GPCR_OFFSET);
-- 
2.1.4

Re: [PATCH 1/4] gpio: pxa: convert to one gpiochip

[Linus Walleij]

On Sat, Nov 28, 2015 at 10:37 PM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:

> The pxa gpio IP is provided by one chip, which holds multiple banks.
>
> Another reason the driver should register only one gpiochip instead of
> multiple gpiochips (ie. 1 per each bank) is that for pincontrol and
> devicetree integration (think gpio-ranges), it's impossible to have the
> contiguous pin range 0..127 mapped to gpios 0..127.
>
> This patch, amongst other thinks, paves the path to loosen the bond with
> the global structure variable pxa_gpio_chip.
>
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>

Patch applied.

Since noone else seems to care, would you consider sending a patch
to MAINTAINERS listing yourself as maintainer of this GPIO driver?

Yours,
Linus Walleij

Re: [PATCH 1/4] gpio: pxa: convert to one gpiochip

[Robert Jarzmik]

Linus Walleij <linus.walleij@linaro.org> writes:

> On Sat, Nov 28, 2015 at 10:37 PM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
>
>> The pxa gpio IP is provided by one chip, which holds multiple banks.
>>
>> Another reason the driver should register only one gpiochip instead of
>> multiple gpiochips (ie. 1 per each bank) is that for pincontrol and
>> devicetree integration (think gpio-ranges), it's impossible to have the
>> contiguous pin range 0..127 mapped to gpios 0..127.
>>
>> This patch, amongst other thinks, paves the path to loosen the bond with
>> the global structure variable pxa_gpio_chip.
>>
>> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
>
> Patch applied.
>
> Since noone else seems to care, would you consider sending a patch
> to MAINTAINERS listing yourself as maintainer of this GPIO driver?
>
> Yours,
> Linus Walleij

But of course, I'll do it with pleasure right after work :)

Cheers.

-- 
Robert

[PATCH 2/4] gpio: pxa: convert to devm_ioremap

[Robert Jarzmik]

Use the device managed ioremap to simplify the probe function.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/gpio/gpio-pxa.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-pxa.c b/drivers/gpio/gpio-pxa.c
index de2cfbeccaa1..8558abf98204 100644
--- a/drivers/gpio/gpio-pxa.c
+++ b/drivers/gpio/gpio-pxa.c
@@ -614,9 +614,8 @@ static int pxa_gpio_probe(struct platform_device *pdev)
 		|| (irq_mux <= 0))
 		return -EINVAL;
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res)
-		return -EINVAL;
-	gpio_reg_base = ioremap(res->start, resource_size(res));
+	gpio_reg_base = devm_ioremap(&pdev->dev, res->start,
+				     resource_size(res));
 	if (!gpio_reg_base)
 		return -EINVAL;
 
@@ -627,13 +626,11 @@ static int pxa_gpio_probe(struct platform_device *pdev)
 	if (IS_ERR(clk)) {
 		dev_err(&pdev->dev, "Error %ld to get gpio clock\n",
 			PTR_ERR(clk));
-		iounmap(gpio_reg_base);
 		return PTR_ERR(clk);
 	}
 	ret = clk_prepare_enable(clk);
 	if (ret) {
 		clk_put(clk);
-		iounmap(gpio_reg_base);
 		return ret;
 	}
 
-- 
2.1.4

Re: [PATCH 2/4] gpio: pxa: convert to devm_ioremap

[Linus Walleij]

On Sat, Nov 28, 2015 at 10:37 PM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:

> Use the device managed ioremap to simplify the probe function.
>
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>

Patch applied.

Yours,
Linus Walleij

[PATCH 3/4] gpio: pxa: change the interrupt management

[Robert Jarzmik]

The interrupt management is changed by this patch to rely on chip data
instead of chained interrupts.

The main goal is to loosen the dependency on the global pxa chip
structure in favor of the passed chip data. The secondary goal is to
better show in /proc/interrupts the difference between interrupts for
gpio0 and gpio1 (directly wired to interrupt controller), and the other
gpios (wired onto a third line in the interrupt controller).

The last advantage of this patch is that the interrupt is actually
requested, so that another driver cannot steal this line, or overwrite
the handler.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/gpio/gpio-pxa.c | 145 +++++++++++++++++++++++++++---------------------
 1 file changed, 82 insertions(+), 63 deletions(-)

diff --git a/drivers/gpio/gpio-pxa.c b/drivers/gpio/gpio-pxa.c
index 8558abf98204..7e02157c5b92 100644
--- a/drivers/gpio/gpio-pxa.c
+++ b/drivers/gpio/gpio-pxa.c
@@ -64,11 +64,6 @@
 int pxa_last_gpio;
 static int irq_base;
 
-#ifdef CONFIG_OF
-static struct irq_domain *domain;
-static struct device_node *pxa_gpio_of_node;
-#endif
-
 struct pxa_gpio_bank {
 	void __iomem	*regbase;
 	unsigned long	irq_mask;
@@ -87,6 +82,7 @@ struct pxa_gpio_chip {
 	struct device *dev;
 	struct gpio_chip chip;
 	struct pxa_gpio_bank *banks;
+	struct irq_domain *irqdomain;
 
 	int irq0;
 	int irq1;
@@ -231,14 +227,23 @@ static inline int __gpio_is_occupied(struct pxa_gpio_chip *pchip, unsigned gpio)
 	return ret;
 }
 
-static int pxa_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
+int pxa_irq_to_gpio(int irq)
 {
-	return offset + irq_base;
+	struct pxa_gpio_chip *pchip = pxa_gpio_chip;
+	int irq_gpio0;
+
+	irq_gpio0 = irq_find_mapping(pchip->irqdomain, 0);
+	if (irq_gpio0 > 0)
+		return irq - irq_gpio0;
+
+	return irq_gpio0;
 }
 
-int pxa_irq_to_gpio(int irq)
+static int pxa_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
 {
-	return irq - irq_base;
+	struct pxa_gpio_chip *pchip = chip_to_pxachip(chip);
+
+	return irq_find_mapping(pchip->irqdomain, offset);
 }
 
 static int pxa_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
@@ -314,7 +319,7 @@ static int pxa_gpio_of_xlate(struct gpio_chip *gc,
 #endif
 
 static int pxa_init_gpio_chip(struct pxa_gpio_chip *pchip, int ngpio,
-			      void __iomem *regbase)
+			      struct device_node *np, void __iomem *regbase)
 {
 	int i, gpio, nbanks = DIV_ROUND_UP(ngpio, 32);
 	struct pxa_gpio_bank *bank;
@@ -332,7 +337,7 @@ static int pxa_init_gpio_chip(struct pxa_gpio_chip *pchip, int ngpio,
 	pchip->chip.to_irq = pxa_gpio_to_irq;
 	pchip->chip.ngpio = ngpio;
 #ifdef CONFIG_OF_GPIO
-	pchip->chip.of_node = pxa_gpio_of_node;
+	pchip->chip.of_node = np;
 	pchip->chip.of_xlate = pxa_gpio_of_xlate;
 	pchip->chip.of_gpio_n_cells = 2;
 #endif
@@ -362,8 +367,8 @@ static inline void update_edge_detect(struct pxa_gpio_bank *c)
 
 static int pxa_gpio_irq_type(struct irq_data *d, unsigned int type)
 {
-	struct pxa_gpio_chip *pchip = pxa_gpio_chip;
-	int gpio = pxa_irq_to_gpio(d->irq);
+	struct pxa_gpio_chip *pchip = irq_data_get_irq_chip_data(d);
+	unsigned int gpio = irqd_to_hwirq(d);
 	struct pxa_gpio_bank *c = gpio_to_pxabank(&pchip->chip, gpio);
 	unsigned long gpdr, mask = GPIO_bit(gpio);
 
@@ -405,16 +410,13 @@ static int pxa_gpio_irq_type(struct irq_data *d, unsigned int type)
 	return 0;
 }
 
-static void pxa_gpio_demux_handler(struct irq_desc *desc)
+static irqreturn_t pxa_gpio_demux_handler(int in_irq, void *d)
 {
 	int loop, gpio, n, handled = 0;
 	unsigned long gedr;
-	struct irq_chip *chip = irq_desc_get_chip(desc);
-	struct pxa_gpio_chip *pchip = pxa_gpio_chip;
+	struct pxa_gpio_chip *pchip = d;
 	struct pxa_gpio_bank *c;
 
-	chained_irq_enter(chip, desc);
-
 	do {
 		loop = 0;
 		for_each_gpio_bank(gpio, c, pchip) {
@@ -428,15 +430,31 @@ static void pxa_gpio_demux_handler(struct irq_desc *desc)
 				generic_handle_irq(gpio_to_irq(gpio + n));
 			}
 		}
+		handled += loop;
 	} while (loop);
 
-	chained_irq_exit(chip, desc);
+	return handled ? IRQ_HANDLED : IRQ_NONE;
+}
+
+static irqreturn_t pxa_gpio_direct_handler(int in_irq, void *d)
+{
+	struct pxa_gpio_chip *pchip = d;
+
+	if (in_irq == pchip->irq0) {
+		generic_handle_irq(gpio_to_irq(0));
+	} else if (in_irq == pchip->irq1) {
+		generic_handle_irq(gpio_to_irq(1));
+	} else {
+		pr_err("%s() unknown irq %d\n", __func__, in_irq);
+		return IRQ_NONE;
+	}
+	return IRQ_HANDLED;
 }
 
 static void pxa_ack_muxed_gpio(struct irq_data *d)
 {
-	struct pxa_gpio_chip *pchip = pxa_gpio_chip;
-	int gpio = pxa_irq_to_gpio(d->irq);
+	struct pxa_gpio_chip *pchip = irq_data_get_irq_chip_data(d);
+	unsigned int gpio = irqd_to_hwirq(d);
 	void __iomem *base = gpio_bank_base(&pchip->chip, gpio);
 
 	writel_relaxed(GPIO_bit(gpio), base + GEDR_OFFSET);
@@ -444,8 +462,8 @@ static void pxa_ack_muxed_gpio(struct irq_data *d)
 
 static void pxa_mask_muxed_gpio(struct irq_data *d)
 {
-	struct pxa_gpio_chip *pchip = pxa_gpio_chip;
-	int gpio = pxa_irq_to_gpio(d->irq);
+	struct pxa_gpio_chip *pchip = irq_data_get_irq_chip_data(d);
+	unsigned int gpio = irqd_to_hwirq(d);
 	struct pxa_gpio_bank *b = gpio_to_pxabank(&pchip->chip, gpio);
 	void __iomem *base = gpio_bank_base(&pchip->chip, gpio);
 	uint32_t grer, gfer;
@@ -460,8 +478,8 @@ static void pxa_mask_muxed_gpio(struct irq_data *d)
 
 static int pxa_gpio_set_wake(struct irq_data *d, unsigned int on)
 {
-	int gpio = pxa_irq_to_gpio(d->irq);
-	struct pxa_gpio_chip *pchip = pxa_gpio_chip;
+	struct pxa_gpio_chip *pchip = irq_data_get_irq_chip_data(d);
+	unsigned int gpio = irqd_to_hwirq(d);
 
 	if (pchip->set_wake)
 		return pchip->set_wake(gpio, on);
@@ -471,8 +489,8 @@ static int pxa_gpio_set_wake(struct irq_data *d, unsigned int on)
 
 static void pxa_unmask_muxed_gpio(struct irq_data *d)
 {
-	struct pxa_gpio_chip *pchip = pxa_gpio_chip;
-	int gpio = pxa_irq_to_gpio(d->irq);
+	struct pxa_gpio_chip *pchip = irq_data_get_irq_chip_data(d);
+	unsigned int gpio = irqd_to_hwirq(d);
 	struct pxa_gpio_bank *c = gpio_to_pxabank(&pchip->chip, gpio);
 
 	c->irq_mask |= GPIO_bit(gpio);
@@ -531,6 +549,7 @@ static int pxa_irq_domain_map(struct irq_domain *d, unsigned int irq,
 {
 	irq_set_chip_and_handler(irq, &pxa_muxed_gpio_chip,
 				 handle_edge_irq);
+	irq_set_chip_data(irq, d->host_data);
 	irq_set_noprobe(irq);
 	return 0;
 }
@@ -544,7 +563,6 @@ static int pxa_gpio_probe_dt(struct platform_device *pdev,
 			     struct pxa_gpio_chip *pchip)
 {
 	int nr_gpios;
-	struct device_node *np = pdev->dev.of_node;
 	const struct of_device_id *of_id =
 				of_match_device(pxa_gpio_dt_ids, &pdev->dev);
 	const struct pxa_gpio_id *gpio_id;
@@ -564,10 +582,7 @@ static int pxa_gpio_probe_dt(struct platform_device *pdev,
 		dev_err(&pdev->dev, "Failed to allocate IRQ numbers\n");
 		return irq_base;
 	}
-	domain = irq_domain_add_legacy(np, nr_gpios, irq_base, 0,
-				       &pxa_irq_domain_ops, pchip);
-	pxa_gpio_of_node = np;
-	return 0;
+	return irq_base;
 }
 #else
 #define pxa_gpio_probe_dt(pdev, pchip)		(-1)
@@ -581,7 +596,7 @@ static int pxa_gpio_probe(struct platform_device *pdev)
 	struct clk *clk;
 	struct pxa_gpio_platform_data *info;
 	void __iomem *gpio_reg_base;
-	int gpio, irq, ret, use_of = 0;
+	int gpio, ret;
 	int irq0 = 0, irq1 = 0, irq_mux, gpio_offset = 0;
 
 	pchip = devm_kzalloc(&pdev->dev, sizeof(*pchip), GFP_KERNEL);
@@ -597,22 +612,29 @@ static int pxa_gpio_probe(struct platform_device *pdev)
 		pxa_last_gpio = pxa_gpio_nums(pdev);
 		pchip->set_wake = info->gpio_set_wake;
 	} else {
-		irq_base = 0;
-		use_of = 1;
-		ret = pxa_gpio_probe_dt(pdev, pchip);
-		if (ret < 0)
+		irq_base = pxa_gpio_probe_dt(pdev, pchip);
+		if (irq_base < 0)
 			return -EINVAL;
 	}
 
 	if (!pxa_last_gpio)
 		return -EINVAL;
 
+	pchip->irqdomain = irq_domain_add_legacy(pdev->dev.of_node,
+						 pxa_last_gpio + 1, irq_base,
+						 0, &pxa_irq_domain_ops, pchip);
+	if (IS_ERR(pchip->irqdomain))
+		return PTR_ERR(pchip->irqdomain);
+
 	irq0 = platform_get_irq_byname(pdev, "gpio0");
 	irq1 = platform_get_irq_byname(pdev, "gpio1");
 	irq_mux = platform_get_irq_byname(pdev, "gpio_mux");
 	if ((irq0 > 0 && irq1 <= 0) || (irq0 <= 0 && irq1 > 0)
 		|| (irq_mux <= 0))
 		return -EINVAL;
+
+	pchip->irq0 = irq0;
+	pchip->irq1 = irq1;
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	gpio_reg_base = devm_ioremap(&pdev->dev, res->start,
 				     resource_size(res));
@@ -635,7 +657,8 @@ static int pxa_gpio_probe(struct platform_device *pdev)
 	}
 
 	/* Initialize GPIO chips */
-	ret = pxa_init_gpio_chip(pchip, pxa_last_gpio + 1, gpio_reg_base);
+	ret = pxa_init_gpio_chip(pchip, pxa_last_gpio + 1, pdev->dev.of_node,
+				 gpio_reg_base);
 	if (ret) {
 		clk_put(clk);
 		return ret;
@@ -651,35 +674,31 @@ static int pxa_gpio_probe(struct platform_device *pdev)
 			writel_relaxed(~0, c->regbase + ED_MASK_OFFSET);
 	}
 
-	if (!use_of) {
-		if (irq0 > 0) {
-			irq = gpio_to_irq(0);
-			irq_set_chip_and_handler(irq, &pxa_muxed_gpio_chip,
-						 handle_edge_irq);
-			irq_clear_status_flags(irq, IRQ_NOREQUEST | IRQ_NOPROBE);
-		}
-		if (irq1 > 0) {
-			irq = gpio_to_irq(1);
-			irq_set_chip_and_handler(irq, &pxa_muxed_gpio_chip,
-						 handle_edge_irq);
-			irq_clear_status_flags(irq, IRQ_NOREQUEST | IRQ_NOPROBE);
-		}
-
-		for (irq  = gpio_to_irq(gpio_offset);
-			irq <= gpio_to_irq(pxa_last_gpio); irq++) {
-			irq_set_chip_and_handler(irq, &pxa_muxed_gpio_chip,
-						 handle_edge_irq);
-			irq_clear_status_flags(irq, IRQ_NOREQUEST | IRQ_NOPROBE);
-		}
+	if (irq0 > 0) {
+		ret = devm_request_irq(&pdev->dev,
+				       irq0, pxa_gpio_direct_handler, 0,
+				       "gpio-0", pchip);
+		if (ret)
+			dev_err(&pdev->dev, "request of gpio0 irq failed: %d\n",
+				ret);
 	}
+	if (irq1 > 0) {
+		ret = devm_request_irq(&pdev->dev,
+				       irq1, pxa_gpio_direct_handler, 0,
+				       "gpio-1", pchip);
+		if (ret)
+			dev_err(&pdev->dev, "request of gpio1 irq failed: %d\n",
+				ret);
+	}
+	ret = devm_request_irq(&pdev->dev,
+			       irq_mux, pxa_gpio_demux_handler, 0,
+				       "gpio-mux", pchip);
+	if (ret)
+		dev_err(&pdev->dev, "request of gpio-mux irq failed: %d\n",
+				ret);
 
-	if (irq0 > 0)
-		irq_set_chained_handler(irq0, pxa_gpio_demux_handler);
-	if (irq1 > 0)
-		irq_set_chained_handler(irq1, pxa_gpio_demux_handler);
 	pxa_gpio_chip = pchip;
 
-	irq_set_chained_handler(irq_mux, pxa_gpio_demux_handler);
 	return 0;
 }
 
-- 
2.1.4

Re: [PATCH 3/4] gpio: pxa: change the interrupt management

[kbuild test robot]

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

Hi Robert,

[auto build test ERROR on: gpio/for-next]
[also build test ERROR on: v4.4-rc2 next-20151127]

url:    https://github.com/0day-ci/linux/commits/Robert-Jarzmik/gpio-pxa-integrate-with-pincontrol/20151129-054738
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next
config: arm-pxa910_defconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers/gpio/gpio-pxa.c: In function 'pxa_gpio_probe':
>> drivers/gpio/gpio-pxa.c:625:12: error: 'pxa_irq_domain_ops' undeclared (first use in this function)
           0, &pxa_irq_domain_ops, pchip);
               ^
   drivers/gpio/gpio-pxa.c:625:12: note: each undeclared identifier is reported only once for each function it appears in
   drivers/gpio/gpio-pxa.c: At top level:
   drivers/gpio/gpio-pxa.c:500:24: warning: 'pxa_muxed_gpio_chip' defined but not used [-Wunused-variable]
    static struct irq_chip pxa_muxed_gpio_chip = {
                           ^

vim +/pxa_irq_domain_ops +625 drivers/gpio/gpio-pxa.c

   619	
   620		if (!pxa_last_gpio)
   621			return -EINVAL;
   622	
   623		pchip->irqdomain = irq_domain_add_legacy(pdev->dev.of_node,
   624							 pxa_last_gpio + 1, irq_base,
 > 625							 0, &pxa_irq_domain_ops, pchip);
   626		if (IS_ERR(pchip->irqdomain))
   627			return PTR_ERR(pchip->irqdomain);
   628	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 10702 bytes --]

Re: [PATCH 3/4] gpio: pxa: change the interrupt management

[Robert Jarzmik]

kbuild test robot <lkp@intel.com> writes:

> Hi Robert,
>
> [auto build test ERROR on: gpio/for-next]
> [also build test ERROR on: v4.4-rc2 next-20151127]
>
> url:    https://github.com/0day-ci/linux/commits/Robert-Jarzmik/gpio-pxa-integrate-with-pincontrol/20151129-054738
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next
> config: arm-pxa910_defconfig (attached as .config)
> reproduce:
>         wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=arm 
>
> All errors (new ones prefixed by >>):
>
>    drivers/gpio/gpio-pxa.c: In function 'pxa_gpio_probe':
>>> drivers/gpio/gpio-pxa.c:625:12: error: 'pxa_irq_domain_ops' undeclared (first use in this function)
>            0, &pxa_irq_domain_ops, pchip);
>                ^
>    drivers/gpio/gpio-pxa.c:625:12: note: each undeclared identifier is reported only once for each function it appears in
>    drivers/gpio/gpio-pxa.c: At top level:
>    drivers/gpio/gpio-pxa.c:500:24: warning: 'pxa_muxed_gpio_chip' defined but not used [-Wunused-variable]
>     static struct irq_chip pxa_muxed_gpio_chip = {

Ok, that one is a matter of "#ifdef CONFIG_OF" englobbing pxa_irq_domain_ops and
pxa_irq_domain_map().

Didn't see it as the platform_data based test has CONFIG_OF activated in my
kernel. That will be for v2, and tested without the CONFIG_OF to be sure.

Cheers.

--
Robert

Re: [PATCH 3/4] gpio: pxa: change the interrupt management

[Linus Walleij]

On Sat, Nov 28, 2015 at 10:37 PM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:

> The interrupt management is changed by this patch to rely on chip data
> instead of chained interrupts.
>
> The main goal is to loosen the dependency on the global pxa chip
> structure in favor of the passed chip data. The secondary goal is to
> better show in /proc/interrupts the difference between interrupts for
> gpio0 and gpio1 (directly wired to interrupt controller), and the other
> gpios (wired onto a third line in the interrupt controller).
>
> The last advantage of this patch is that the interrupt is actually
> requested, so that another driver cannot steal this line, or overwrite
> the handler.
>
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>

I stopped applying here, waiting for a v2 of the two last
patches.

Yours,
Linus Walleij

[PATCH 4/4] gpio: pxa: add pin control gpio direction and request

[Robert Jarzmik]

If a pin control driver is available, use it to change the gpio
direction. If not fallback to directly manipulating the gpio direction
register.

The reason to use the pin control driver first is that pin control in
pxa2xx architecture implies changing the gpio direction, even for non
gpio functions. In order to do it atomically, only one driver should
control the gpio direction, and if a pin controller is available, it has
to be him.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/gpio/gpio-pxa.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/gpio/gpio-pxa.c b/drivers/gpio/gpio-pxa.c
index 7e02157c5b92..01d507502d56 100644
--- a/drivers/gpio/gpio-pxa.c
+++ b/drivers/gpio/gpio-pxa.c
@@ -24,6 +24,7 @@
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/syscore_ops.h>
 #include <linux/slab.h>
@@ -251,6 +252,11 @@ static int pxa_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
 	void __iomem *base = gpio_bank_base(chip, offset);
 	uint32_t value, mask = GPIO_bit(offset);
 	unsigned long flags;
+	int ret;
+
+	ret = pinctrl_gpio_direction_input(chip->base + offset);
+	if (!ret)
+		return 0;
 
 	spin_lock_irqsave(&gpio_lock, flags);
 
@@ -271,9 +277,14 @@ static int pxa_gpio_direction_output(struct gpio_chip *chip,
 	void __iomem *base = gpio_bank_base(chip, offset);
 	uint32_t tmp, mask = GPIO_bit(offset);
 	unsigned long flags;
+	int ret;
 
 	writel_relaxed(mask, base + (value ? GPSR_OFFSET : GPCR_OFFSET));
 
+	ret = pinctrl_gpio_direction_output(chip->base + offset);
+	if (!ret)
+		return 0;
+
 	spin_lock_irqsave(&gpio_lock, flags);
 
 	tmp = readl_relaxed(base + GPDR_OFFSET);
@@ -318,6 +329,16 @@ static int pxa_gpio_of_xlate(struct gpio_chip *gc,
 }
 #endif
 
+static int pxa_gpio_request(struct gpio_chip *chip, unsigned int offset)
+{
+	return pinctrl_request_gpio(chip->base + offset);
+}
+
+static void pxa_gpio_free(struct gpio_chip *chip, unsigned int offset)
+{
+	pinctrl_free_gpio(chip->base + offset);
+}
+
 static int pxa_init_gpio_chip(struct pxa_gpio_chip *pchip, int ngpio,
 			      struct device_node *np, void __iomem *regbase)
 {
@@ -336,6 +357,8 @@ static int pxa_init_gpio_chip(struct pxa_gpio_chip *pchip, int ngpio,
 	pchip->chip.set = pxa_gpio_set;
 	pchip->chip.to_irq = pxa_gpio_to_irq;
 	pchip->chip.ngpio = ngpio;
+	pchip->chip.request = pxa_gpio_request;
+	pchip->chip.free = pxa_gpio_free;
 #ifdef CONFIG_OF_GPIO
 	pchip->chip.of_node = np;
 	pchip->chip.of_xlate = pxa_gpio_of_xlate;
-- 
2.1.4

Re: [PATCH 4/4] gpio: pxa: add pin control gpio direction and request

[Robert Jarzmik]

Robert Jarzmik <robert.jarzmik@free.fr> writes:

> +static int pxa_gpio_request(struct gpio_chip *chip, unsigned int offset)
> +{
> +	return pinctrl_request_gpio(chip->base + offset);
> +}
> +
> +static void pxa_gpio_free(struct gpio_chip *chip, unsigned int offset)
> +{
> +	pinctrl_free_gpio(chip->base + offset);
> +}

This part will need a bit of rework : in the case of a platform data board,
pinctrl_*() returns -EPROBE_DEFER, preventing any gpio user to claim a gpio.

Either I hold with this patch to see how pxa pinctrl goes, or drop it, but in
any case this one is not ready yet for merge.

Cheers.

-- 
Robert

Re: [PATCH 0/4] gpio: pxa: integrate with pincontrol

[Linus Walleij]

On Sat, Nov 28, 2015 at 10:37 PM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:

> Hi Linus, Alexandre and Haojian,
>
> This serie aims at several cleanups and improvements in the pxa gpio driver, to

I have concerns about this series.

I am worried that joining the banks into one gpio_chip makes it
impossible for you GPIOLIB_IRQCHIP. Usually that is possible and
preferrable when using a chained handler if e.g. one bank has
one IRQ line.

But overall that depends on how the IRQs map on this hardware.
Can you describe how the GPIO IRQs work on the PXA27x?

Yours,
Linus Walleij

Re: [PATCH 0/4] gpio: pxa: integrate with pincontrol

[Robert Jarzmik]

Linus Walleij <linus.walleij@linaro.org> writes:

> On Sat, Nov 28, 2015 at 10:37 PM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
>
>> Hi Linus, Alexandre and Haojian,
>>
>> This serie aims at several cleanups and improvements in the pxa gpio driver, to
>
> I have concerns about this series.
>
> I am worried that joining the banks into one gpio_chip makes it
> impossible for you GPIOLIB_IRQCHIP. Usually that is possible and
> preferrable when using a chained handler if e.g. one bank has
> one IRQ line.
>
> But overall that depends on how the IRQs map on this hardware.
> Can you describe how the GPIO IRQs work on the PXA27x?
Of course.

For PXA27x, there are 3 interrupts directly connected to the CPU of the SoC,
ie. the primary irq controller :
 - one is only triggered if GPIO0 has a rising/falling edge
 - one is only triggered if GPIO1 has a rising/falling edge
 - the last is triggered if any GPIOn has a rising/falling edge (n >= 2)

The condition to program the rising/falling edge which implies the interrupt to
be asserted is in a GPIO block register, GFER and GRER (1 bit per GPIO).

The fact that the last interrupt (let's call it gpiomux_irq) is triggered by
GPIOs from _all_ the banks makes me believe it's a single IP block, ie. a single
chip.

Now if you have concerns with this, then maybe you can advise another approach,
I'm pretty open. The final goal will be for me :
 - gpio and pinctrl have to cooperate
   - today, with the current state, it's impossible to map pins 0..127 to gpios
     0..127, at least in a device-tree .dts file
   - the GPDR (gpio direction register) shared access bothers me a bit

Cheers.

-- 
Robert

Re: [PATCH 0/4] gpio: pxa: integrate with pincontrol

[Linus Walleij]

On Thu, Dec 10, 2015 at 8:28 AM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
> Linus Walleij <linus.walleij@linaro.org> writes:
>
>> On Sat, Nov 28, 2015 at 10:37 PM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
>>
>>> Hi Linus, Alexandre and Haojian,
>>>
>>> This serie aims at several cleanups and improvements in the pxa gpio driver, to
>>
>> I have concerns about this series.
>>
>> I am worried that joining the banks into one gpio_chip makes it
>> impossible for you GPIOLIB_IRQCHIP. Usually that is possible and
>> preferrable when using a chained handler if e.g. one bank has
>> one IRQ line.
>>
>> But overall that depends on how the IRQs map on this hardware.
>> Can you describe how the GPIO IRQs work on the PXA27x?
> Of course.
>
> For PXA27x, there are 3 interrupts directly connected to the CPU of the SoC,
> ie. the primary irq controller :
>  - one is only triggered if GPIO0 has a rising/falling edge
>  - one is only triggered if GPIO1 has a rising/falling edge
>  - the last is triggered if any GPIOn has a rising/falling edge (n >= 2)
>
> The condition to program the rising/falling edge which implies the interrupt to
> be asserted is in a GPIO block register, GFER and GRER (1 bit per GPIO).
>
> The fact that the last interrupt (let's call it gpiomux_irq) is triggered by
> GPIOs from _all_ the banks makes me believe it's a single IP block, ie. a single
> chip.

OK you're probably right. So GPIO0 and 1 are special cases and the
rest a muxed GPIO case. That's sufficiently odd to warrant its own
irqdomain and not use GPIOLIB_IRQCHIP.

I guess I will go ahead and merge this, simply.

> Now if you have concerns with this, then maybe you can advise another approach,
> I'm pretty open. The final goal will be for me :
>  - gpio and pinctrl have to cooperate
>    - today, with the current state, it's impossible to map pins 0..127 to gpios
>      0..127, at least in a device-tree .dts file

OK sounds good.

>    - the GPDR (gpio direction register) shared access bothers me a bit

How is it shared and between what users?

Yours,
Linus Walleij

Re: [PATCH 0/4] gpio: pxa: integrate with pincontrol

[Robert Jarzmik]

Linus Walleij <linus.walleij@linaro.org> writes:

>>    - the GPDR (gpio direction register) shared access bothers me a bit
>
> How is it shared and between what users?
It's shared between the pin controller and the gpio controller.

The odd thing with the pxa architecture is that the GPDR bit selects between 2
different alternate functions, even when the pin is not a GPIO. Strange design,
isn't it ?

As a consequence, both the gpio driver and pinctrl have to modify it, for
different purposes :
 - pinctrl will modify it to select a specific alternate function
 - gpio driver will modify it when the pin is a GPIO, to modify its direction.

Cheers.

-- 
Robert

Re: [PATCH 0/4] gpio: pxa: integrate with pincontrol

[Linus Walleij]

On Thu, Dec 10, 2015 at 6:31 PM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
> Linus Walleij <linus.walleij@linaro.org> writes:
>
>>>    - the GPDR (gpio direction register) shared access bothers me a bit
>>
>> How is it shared and between what users?
>
> It's shared between the pin controller and the gpio controller.

OK then it may be one of these cases where we should jit the pin controller
and the GPIO controller together in the same file (under drivers/pinctrl)
to simplify the mess. We can do that in the NEXT merge window because
right now I don't want any more crisscross between gpio and pin control
as there are refactorings I'm piling up.

Another option is e.g. accessing the registers through regmap-mmio but
it feels a bit like overkill for this...

> The odd thing with the pxa architecture is that the GPDR bit selects between 2
> different alternate functions, even when the pin is not a GPIO. Strange design,
> isn't it ?

Probably just unfortunate naming.

In my presentation "building GPIO and pin control from the ground up" I
try to explain a bit how hardware engineers design these things...
http://dflund.se/~triad/papers/pincontrol.pdf

> As a consequence, both the gpio driver and pinctrl have to modify it, for
> different purposes :
>  - pinctrl will modify it to select a specific alternate function
>  - gpio driver will modify it when the pin is a GPIO, to modify its direction.

OK. Solutions per above, I guess it currently just optimistically hope
we do not fiddle the same bit in parallell from the two drivers (which
is maybe even possible to prove to be true).

Yours,
Linus Walleij

Re: [PATCH 0/4] gpio: pxa: integrate with pincontrol

[Robert Jarzmik]

Linus Walleij <linus.walleij@linaro.org> writes:

> On Thu, Dec 10, 2015 at 6:31 PM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
>> Linus Walleij <linus.walleij@linaro.org> writes:
>>
>>>>    - the GPDR (gpio direction register) shared access bothers me a bit
>>>
>>> How is it shared and between what users?
>>
>> It's shared between the pin controller and the gpio controller.
>
> OK then it may be one of these cases where we should jit the pin controller
> and the GPIO controller together in the same file (under drivers/pinctrl)
> to simplify the mess. We can do that in the NEXT merge window because
> right now I don't want any more crisscross between gpio and pin control
> as there are refactorings I'm piling up.

Well, maybe, but I don't know how to do it, due to the number of possibilities,
given that :
 - gpio-pxa.c should work for pxa27x device-tree
   (this would be possible with a pinctrl+gpio fusion)
 - gpio-pxa.c should work for pxa3xx device-tree
   (I don't know how to do this, pxa3xx uses pinctrl-single AFAIK)
 - gpio-pxa.c should work for pxa168 + mmp* device-tree
   (same as for pxa3xx)
 - gpio-pxa.c might should with pxa27x platform_data
   (this doesn't work yet fully, the wake-up pin give me headaches)
 - gpio-pxa.c should work with pxa25x platform_data
   (I don't see either how a merged pinctrl+gpio driver could address this)

All of this to say it looks a bit complicated to me to have a gpio+pinctrl in
the same file, but I might be missing something obvious.
 
> Another option is e.g. accessing the registers through regmap-mmio but
> it feels a bit like overkill for this...
Yes, overkill maybe, but maybe an idea. The nice thing about regmap is the debug
capabilities, the less good is the permanent need of locks ...

>> The odd thing with the pxa architecture is that the GPDR bit selects between 2
>> different alternate functions, even when the pin is not a GPIO. Strange design,
>> isn't it ?
>
> Probably just unfortunate naming.
>
> In my presentation "building GPIO and pin control from the ground up" I
> try to explain a bit how hardware engineers design these things...
> http://dflund.se/~triad/papers/pincontrol.pdf
Nice, I had already read it :)

>> As a consequence, both the gpio driver and pinctrl have to modify it, for
>> different purposes :
>>  - pinctrl will modify it to select a specific alternate function
>>  - gpio driver will modify it when the pin is a GPIO, to modify its direction.
>
> OK. Solutions per above, I guess it currently just optimistically hope
> we do not fiddle the same bit in parallell from the two drivers (which
> is maybe even possible to prove to be true).
Ok. I will think about it again in the next days.

Cheers.

-- 
Robert