[PATCH-next 00/20] gpio: gpio-omap: set of fixes and big clean-up

[PATCH-next 00/20] gpio: gpio-omap: set of fixes and big clean-up

[Grygorii Strashko]

Hi Linus, Russell, Tony, All,

This series contains set of patches from Russell King which were circulated
internally for quite some time already and I fill it's reasonable to move
future discussion upstream (and also avoid rebasing).
Fisrt two patches are fixes and the rest are big, great clean up
from Russell King.

Personally, I like this clean up and refactoring very much and don't want
it to be lost.

Code can be found at:
 git@git.ti.com:~gragst/ti-linux-kernel/gragsts-ti-linux-kernel.git
branch:
 lkml-next-gpio-clean-up

Russell King (20):
  gpio: gpio-omap: ensure irq is enabled before wakeup
  gpio: gpio-omap: fix lack of irqstatus_raw0 for OMAP4
  gpio: gpio-omap: remove remainder of list management
  gpio: gpio-omap: clean up edge interrupt handling
  gpio: gpio-omap: remove irq_ack method
  gpio: gpio-omap: move omap_gpio_request() and omap_gpio_free()
  gpio: gpio-omap: simplify omap_gpio_get_direction()
  gpio: gpio-omap: simplify get() method
  gpio: gpio-omap: simplify get_multiple()
  gpio: gpio-omap: simplify set_multiple()
  gpio: gpio-omap: simplify bank->level_mask
  gpio: gpio-omap: simplify read-modify-write
  gpio: gpio-omap: simplify omap_toggle_gpio_edge_triggering()
  gpio: gpio-omap: simplify omap_set_gpio_irqenable()
  gpio: gpio-omap: remove dataout variation in context handling
  gpio: gpio-omap: clean up omap_gpio_restore_context()
  gpio: gpio-omap: constify register tables
  gpio: gpio-omap: clean up wakeup handling
  gpio: gpio-omap: irq_startup() must not return error codes
  gpio: gpio-omap: clean up register access in omap2_set_gpio_debounce()

 drivers/gpio/gpio-omap.c                | 497 ++++++++----------------
 include/linux/platform_data/gpio-omap.h |   2 +-
 2 files changed, 161 insertions(+), 338 deletions(-)

-- 
2.17.1

[PATCH-next 01/20] gpio: gpio-omap: ensure irq is enabled before wakeup

[Grygorii Strashko]

From: Russell King <rmk+kernel@armlinux.org.uk>

Documentation states:

  NOTE: There must be a correlation between the wake-up enable and
  interrupt-enable registers. If a GPIO pin has a wake-up configured
  on it, it must also have the corresponding interrupt enabled (on
  one of the two interrupt lines).

Ensure that this condition is always satisfied by enabling the detection
events after enabling the interrupt, and disabling the detection before
disabling the interrupt.  This ensures interrupt/wakeup events can not
happen until both the wakeup and interrupt enables correlate.

If we do any clearing, clear between the interrupt enable/disable and
trigger setting.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/gpio/gpio-omap.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 16289bafa001..2c6d46396834 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -832,9 +832,9 @@ static void omap_gpio_irq_shutdown(struct irq_data *d)
 
 	raw_spin_lock_irqsave(&bank->lock, flags);
 	bank->irq_usage &= ~(BIT(offset));
-	omap_set_gpio_irqenable(bank, offset, 0);
-	omap_clear_gpio_irqstatus(bank, offset);
 	omap_set_gpio_triggering(bank, offset, IRQ_TYPE_NONE);
+	omap_clear_gpio_irqstatus(bank, offset);
+	omap_set_gpio_irqenable(bank, offset, 0);
 	if (!LINE_USED(bank->mod_usage, offset))
 		omap_clear_gpio_debounce(bank, offset);
 	omap_disable_gpio_module(bank, offset);
@@ -870,8 +870,8 @@ static void omap_gpio_mask_irq(struct irq_data *d)
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&bank->lock, flags);
-	omap_set_gpio_irqenable(bank, offset, 0);
 	omap_set_gpio_triggering(bank, offset, IRQ_TYPE_NONE);
+	omap_set_gpio_irqenable(bank, offset, 0);
 	raw_spin_unlock_irqrestore(&bank->lock, flags);
 }
 
@@ -883,9 +883,6 @@ static void omap_gpio_unmask_irq(struct irq_data *d)
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&bank->lock, flags);
-	if (trigger)
-		omap_set_gpio_triggering(bank, offset, trigger);
-
 	omap_set_gpio_irqenable(bank, offset, 1);
 
 	/*
@@ -893,9 +890,13 @@ static void omap_gpio_unmask_irq(struct irq_data *d)
 	 * is cleared, thus after the handler has run. OMAP4 needs this done
 	 * after enabing the interrupt to clear the wakeup status.
 	 */
-	if (bank->level_mask & BIT(offset))
+	if (bank->regs->leveldetect0 && bank->regs->wkup_en &&
+	    trigger & (IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))
 		omap_clear_gpio_irqstatus(bank, offset);
 
+	if (trigger)
+		omap_set_gpio_triggering(bank, offset, trigger);
+
 	raw_spin_unlock_irqrestore(&bank->lock, flags);
 }
 
-- 
2.17.1

[PATCH-next 02/20] gpio: gpio-omap: fix lack of irqstatus_raw0 for OMAP4

[Grygorii Strashko]

From: Russell King <rmk+kernel@armlinux.org.uk>

Commit 384ebe1c2849 ("gpio/omap: Add DT support to GPIO driver") added
the register definition tables to the gpio-omap driver. Subsequently to
that commit, commit 4e962e8998cc ("gpio/omap: remove cpu_is_omapxxxx()
checks from *_runtime_resume()") added definitions for irqstatus_raw*
registers to the legacy OMAP4 definitions, but missed the DT
definitions.

This causes an unintentional change of behaviour for the 1.101 errata
workaround on OMAP4 platforms. Fix this oversight.

Fixes: 4e962e8998cc ("gpio/omap: remove cpu_is_omapxxxx() checks from *_runtime_resume()")
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/gpio/gpio-omap.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 2c6d46396834..44b214e5cefb 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1457,6 +1457,8 @@ static struct omap_gpio_reg_offs omap4_gpio_regs = {
 	.clr_dataout =		OMAP4_GPIO_CLEARDATAOUT,
 	.irqstatus =		OMAP4_GPIO_IRQSTATUS0,
 	.irqstatus2 =		OMAP4_GPIO_IRQSTATUS1,
+	.irqstatus_raw0 =	OMAP4_GPIO_IRQSTATUSRAW0,
+	.irqstatus_raw1 =	OMAP4_GPIO_IRQSTATUSRAW1,
 	.irqenable =		OMAP4_GPIO_IRQSTATUSSET0,
 	.irqenable2 =		OMAP4_GPIO_IRQSTATUSSET1,
 	.set_irqenable =	OMAP4_GPIO_IRQSTATUSSET0,
-- 
2.17.1

[PATCH-next 03/20] gpio: gpio-omap: remove remainder of list management

[Grygorii Strashko]

From: Russell King <rmk+kernel@armlinux.org.uk>

Commit c4791bc6e3a6 ("gpio: omap: drop omap_gpio_list") removed the
list head and addition to the list head of each gpio bank, but failed
to remove the list_del() call and the node inside struct gpio_bank.
Remove these too.

Fixes: c4791bc6e3a6 ("gpio: omap: drop omap_gpio_list")
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/gpio/gpio-omap.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 44b214e5cefb..708a135e2f0c 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -47,7 +47,6 @@ struct gpio_regs {
 };
 
 struct gpio_bank {
-	struct list_head node;
 	void __iomem *base;
 	int irq;
 	u32 non_wakeup_gpios;
@@ -1641,7 +1640,6 @@ static int omap_gpio_remove(struct platform_device *pdev)
 	struct gpio_bank *bank = platform_get_drvdata(pdev);
 
 	cpu_pm_unregister_notifier(&bank->nb);
-	list_del(&bank->node);
 	gpiochip_remove(&bank->chip);
 	pm_runtime_disable(&pdev->dev);
 	if (bank->dbck_flag)
-- 
2.17.1

[PATCH-next 04/20] gpio: gpio-omap: clean up edge interrupt handling

[Grygorii Strashko]

From: Russell King <rmk+kernel@armlinux.org.uk>

The edge interrupt handling was effectively:

	isr = ISR_reg & enabled;
	if (bank->level_mask)
		level_mask = bank->level_mask & enabled;
	else
		level_mask = 0;

	edge = isr & ~level_mask;

When bank->level_mask is zero, level_mask will be computed as zero
anyway, so the if() statement is redundant.  We are then left with:

	isr = ISR_reg & enabled;
	level_mask = bank->level_mask & enabled;
	edge = isr & ~level_mask;

This can be simplified further to:

	isr = ISR_reg & enabled;
	edge = isr & ~bank->level_mask;

since the second mask with 'enabled' is redundant.

Improve the associated comment as well.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/gpio/gpio-omap.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 708a135e2f0c..01da1c17bb20 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -733,7 +733,7 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
 static irqreturn_t omap_gpio_irq_handler(int irq, void *gpiobank)
 {
 	void __iomem *isr_reg = NULL;
-	u32 enabled, isr, level_mask;
+	u32 enabled, isr, edge;
 	unsigned int bit;
 	struct gpio_bank *bank = gpiobank;
 	unsigned long wa_lock_flags;
@@ -753,16 +753,14 @@ static irqreturn_t omap_gpio_irq_handler(int irq, void *gpiobank)
 		enabled = omap_get_gpio_irqbank_mask(bank);
 		isr = readl_relaxed(isr_reg) & enabled;
 
-		if (bank->level_mask)
-			level_mask = bank->level_mask & enabled;
-		else
-			level_mask = 0;
-
-		/* clear edge sensitive interrupts before handler(s) are
-		called so that we don't miss any interrupt occurred while
-		executing them */
-		if (isr & ~level_mask)
-			omap_clear_gpio_irqbank(bank, isr & ~level_mask);
+		/*
+		 * Clear edge sensitive interrupts before calling handler(s)
+		 * so subsequent edge transitions are not missed while the
+		 * handlers are running.
+		 */
+		edge = isr & ~bank->level_mask;
+		if (edge)
+			omap_clear_gpio_irqbank(bank, edge);
 
 		raw_spin_unlock_irqrestore(&bank->lock, lock_flags);
 
-- 
2.17.1

[PATCH-next 05/20] gpio: gpio-omap: remove irq_ack method

[Grygorii Strashko]

From: Russell King <rmk+kernel@armlinux.org.uk>

The irq_ack method does not fit our hardware requirements. Edge
interrupts must be cleared before we handle them, and level interrupts
must be cleared after handling them.

We handle the interrupt clearance in our interrupt handler for edge IRQs
and in the unmask method for level IRQs.

Replace the irq_ack method with the no-op method from the dummy irq
chip.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/gpio/gpio-omap.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 01da1c17bb20..04e3da55e39c 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -852,14 +852,6 @@ static void gpio_irq_bus_sync_unlock(struct irq_data *data)
 	pm_runtime_put(bank->chip.parent);
 }
 
-static void omap_gpio_ack_irq(struct irq_data *d)
-{
-	struct gpio_bank *bank = omap_irq_data_get_bank(d);
-	unsigned offset = d->hwirq;
-
-	omap_clear_gpio_irqstatus(bank, offset);
-}
-
 static void omap_gpio_mask_irq(struct irq_data *d)
 {
 	struct gpio_bank *bank = omap_irq_data_get_bank(d);
@@ -1181,11 +1173,8 @@ static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc)
 #endif
 
 	/* MPUIO is a bit different, reading IRQ status clears it */
-	if (bank->is_mpuio) {
-		irqc->irq_ack = dummy_irq_chip.irq_ack;
-		if (!bank->regs->wkup_en)
-			irqc->irq_set_wake = NULL;
-	}
+	if (bank->is_mpuio && !bank->regs->wkup_en)
+		irqc->irq_set_wake = NULL;
 
 	irq = &bank->chip.irq;
 	irq->chip = irqc;
@@ -1531,7 +1520,7 @@ static int omap_gpio_probe(struct platform_device *pdev)
 
 	irqc->irq_startup = omap_gpio_irq_startup,
 	irqc->irq_shutdown = omap_gpio_irq_shutdown,
-	irqc->irq_ack = omap_gpio_ack_irq,
+	irqc->irq_ack = dummy_irq_chip.irq_ack,
 	irqc->irq_mask = omap_gpio_mask_irq,
 	irqc->irq_unmask = omap_gpio_unmask_irq,
 	irqc->irq_set_type = omap_gpio_irq_type,
-- 
2.17.1

[PATCH-next 06/20] gpio: gpio-omap: move omap_gpio_request() and omap_gpio_free()

[Grygorii Strashko]

From: Russell King <rmk+kernel@armlinux.org.uk>

Move these two functions to live beside the rest of the gpio chip
implementation, rather than in the middle of the irq chip
implementation.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/gpio/gpio-omap.c | 64 ++++++++++++++++++++--------------------
 1 file changed, 32 insertions(+), 32 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 04e3da55e39c..cc320d09d074 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -689,38 +689,6 @@ static int omap_gpio_wake_enable(struct irq_data *d, unsigned int enable)
 	return irq_set_irq_wake(bank->irq, enable);
 }
 
-static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
-{
-	struct gpio_bank *bank = gpiochip_get_data(chip);
-	unsigned long flags;
-
-	pm_runtime_get_sync(chip->parent);
-
-	raw_spin_lock_irqsave(&bank->lock, flags);
-	omap_enable_gpio_module(bank, offset);
-	bank->mod_usage |= BIT(offset);
-	raw_spin_unlock_irqrestore(&bank->lock, flags);
-
-	return 0;
-}
-
-static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
-{
-	struct gpio_bank *bank = gpiochip_get_data(chip);
-	unsigned long flags;
-
-	raw_spin_lock_irqsave(&bank->lock, flags);
-	bank->mod_usage &= ~(BIT(offset));
-	if (!LINE_USED(bank->irq_usage, offset)) {
-		omap_set_gpio_direction(bank, offset, 1);
-		omap_clear_gpio_debounce(bank, offset);
-	}
-	omap_disable_gpio_module(bank, offset);
-	raw_spin_unlock_irqrestore(&bank->lock, flags);
-
-	pm_runtime_put(chip->parent);
-}
-
 /*
  * We need to unmask the GPIO bank interrupt as soon as possible to
  * avoid missing GPIO interrupts for other lines in the bank.
@@ -951,6 +919,38 @@ static inline void omap_mpuio_init(struct gpio_bank *bank)
 
 /*---------------------------------------------------------------------*/
 
+static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
+{
+	struct gpio_bank *bank = gpiochip_get_data(chip);
+	unsigned long flags;
+
+	pm_runtime_get_sync(chip->parent);
+
+	raw_spin_lock_irqsave(&bank->lock, flags);
+	omap_enable_gpio_module(bank, offset);
+	bank->mod_usage |= BIT(offset);
+	raw_spin_unlock_irqrestore(&bank->lock, flags);
+
+	return 0;
+}
+
+static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
+{
+	struct gpio_bank *bank = gpiochip_get_data(chip);
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&bank->lock, flags);
+	bank->mod_usage &= ~(BIT(offset));
+	if (!LINE_USED(bank->irq_usage, offset)) {
+		omap_set_gpio_direction(bank, offset, 1);
+		omap_clear_gpio_debounce(bank, offset);
+	}
+	omap_disable_gpio_module(bank, offset);
+	raw_spin_unlock_irqrestore(&bank->lock, flags);
+
+	pm_runtime_put(chip->parent);
+}
+
 static int omap_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
 {
 	struct gpio_bank *bank;
-- 
2.17.1

[PATCH-next 07/20] gpio: gpio-omap: simplify omap_gpio_get_direction()

[Grygorii Strashko]

From: Russell King <rmk+kernel@armlinux.org.uk>

Architectures are single-copy atomic, which means that simply reading
a register is an inherently atomic operation.  There is no need to
take a spinlock here.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/gpio/gpio-omap.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index cc320d09d074..44a4287cce9e 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -953,17 +953,10 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
 
 static int omap_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
 {
-	struct gpio_bank *bank;
-	unsigned long flags;
-	void __iomem *reg;
-	int dir;
+	struct gpio_bank *bank = gpiochip_get_data(chip);
 
-	bank = gpiochip_get_data(chip);
-	reg = bank->base + bank->regs->direction;
-	raw_spin_lock_irqsave(&bank->lock, flags);
-	dir = !!(readl_relaxed(reg) & BIT(offset));
-	raw_spin_unlock_irqrestore(&bank->lock, flags);
-	return dir;
+	return !!(readl_relaxed(bank->base + bank->regs->direction) &
+		  BIT(offset));
 }
 
 static int omap_gpio_input(struct gpio_chip *chip, unsigned offset)
-- 
2.17.1

[PATCH-next 08/20] gpio: gpio-omap: simplify get() method

[Grygorii Strashko]

From: Russell King <rmk+kernel@armlinux.org.uk>

omap_gpio_get() calls omap_get_gpio_datain() or omap_get_gpio_dataout()
to read the GPIO state. These two functions are only called from this
method, so they don't add much value.  Move their contents into
omap_gpio_get() method and simplify.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/gpio/gpio-omap.c | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 44a4287cce9e..bf1e6f1d0de7 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -146,20 +146,6 @@ static void omap_set_gpio_dataout_mask(struct gpio_bank *bank, unsigned offset,
 	bank->context.dataout = l;
 }
 
-static int omap_get_gpio_datain(struct gpio_bank *bank, int offset)
-{
-	void __iomem *reg = bank->base + bank->regs->datain;
-
-	return (readl_relaxed(reg) & (BIT(offset))) != 0;
-}
-
-static int omap_get_gpio_dataout(struct gpio_bank *bank, int offset)
-{
-	void __iomem *reg = bank->base + bank->regs->dataout;
-
-	return (readl_relaxed(reg) & (BIT(offset))) != 0;
-}
-
 /* set multiple data out values using dedicate set/clear register */
 static void omap_set_gpio_dataout_reg_multiple(struct gpio_bank *bank,
 					       unsigned long *mask,
@@ -973,14 +959,15 @@ static int omap_gpio_input(struct gpio_chip *chip, unsigned offset)
 
 static int omap_gpio_get(struct gpio_chip *chip, unsigned offset)
 {
-	struct gpio_bank *bank;
-
-	bank = gpiochip_get_data(chip);
+	struct gpio_bank *bank = gpiochip_get_data(chip);
+	void __iomem *reg;
 
 	if (omap_gpio_is_input(bank, offset))
-		return omap_get_gpio_datain(bank, offset);
+		reg = bank->base + bank->regs->datain;
 	else
-		return omap_get_gpio_dataout(bank, offset);
+		reg = bank->base + bank->regs->dataout;
+
+	return (readl_relaxed(reg) & BIT(offset)) != 0;
 }
 
 static int omap_gpio_output(struct gpio_chip *chip, unsigned offset, int value)
-- 
2.17.1

[PATCH-next 09/20] gpio: gpio-omap: simplify get_multiple()

[Grygorii Strashko]

From: Russell King <rmk+kernel@armlinux.org.uk>

There is no reason to have helper functions to read the datain and
dataout registers when they are only used in one location.  Simplify
this code to make it more readable.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/gpio/gpio-omap.c | 36 +++++++++++-------------------------
 1 file changed, 11 insertions(+), 25 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index bf1e6f1d0de7..a26decc5c611 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -175,22 +175,6 @@ static void omap_set_gpio_dataout_mask_multiple(struct gpio_bank *bank,
 	bank->context.dataout = l;
 }
 
-static unsigned long omap_get_gpio_datain_multiple(struct gpio_bank *bank,
-					      unsigned long *mask)
-{
-	void __iomem *reg = bank->base + bank->regs->datain;
-
-	return readl_relaxed(reg) & *mask;
-}
-
-static unsigned long omap_get_gpio_dataout_multiple(struct gpio_bank *bank,
-					       unsigned long *mask)
-{
-	void __iomem *reg = bank->base + bank->regs->dataout;
-
-	return readl_relaxed(reg) & *mask;
-}
-
 static inline void omap_gpio_rmw(void __iomem *base, u32 reg, u32 mask, bool set)
 {
 	int l = readl_relaxed(base + reg);
@@ -987,18 +971,20 @@ static int omap_gpio_get_multiple(struct gpio_chip *chip, unsigned long *mask,
 				  unsigned long *bits)
 {
 	struct gpio_bank *bank = gpiochip_get_data(chip);
-	void __iomem *reg = bank->base + bank->regs->direction;
-	unsigned long in = readl_relaxed(reg), l;
+	void __iomem *base = bank->base;
+	u32 direction, m, val = 0;
+
+	direction = readl_relaxed(base + bank->regs->direction);
 
-	*bits = 0;
+	m = direction & *mask;
+	if (m)
+		val |= readl_relaxed(base + bank->regs->datain) & m;
 
-	l = in & *mask;
-	if (l)
-		*bits |= omap_get_gpio_datain_multiple(bank, &l);
+	m = ~direction & *mask;
+	if (m)
+		val |= readl_relaxed(base + bank->regs->dataout) & m;
 
-	l = ~in & *mask;
-	if (l)
-		*bits |= omap_get_gpio_dataout_multiple(bank, &l);
+	*bits = val;
 
 	return 0;
 }
-- 
2.17.1

[PATCH-next 10/20] gpio: gpio-omap: simplify set_multiple()

[Grygorii Strashko]

From: Russell King <rmk+kernel@armlinux.org.uk>

One of the reasons for set_multiple() to exist is to allow multiple
GPIOs on the same chip to be changed simultaneously - see commit
5f42424354f5 ("gpiolib: allow simultaneous setting of multiple GPIO
outputs"):

 - Simultaneous glitch-free setting of multiple pins on any kind of
   parallel bus attached to GPIOs provided they all reside on the
   same chip and bank.

In order for this to work, we should not use the atomic set/clear
registers, but instead read-modify-write the dataout register.  We
already take the spinlock to ensure that happens atomically, so
move the code into the set_multiple() function and kill the two
helper functions.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/gpio/gpio-omap.c | 45 +++++++---------------------------------
 1 file changed, 7 insertions(+), 38 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index a26decc5c611..8fdac6e4a929 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -74,8 +74,6 @@ struct gpio_bank {
 	int context_loss_count;
 
 	void (*set_dataout)(struct gpio_bank *bank, unsigned gpio, int enable);
-	void (*set_dataout_multiple)(struct gpio_bank *bank,
-				     unsigned long *mask, unsigned long *bits);
 	int (*get_context_loss_count)(struct device *dev);
 
 	struct omap_gpio_reg_offs *regs;
@@ -146,35 +144,6 @@ static void omap_set_gpio_dataout_mask(struct gpio_bank *bank, unsigned offset,
 	bank->context.dataout = l;
 }
 
-/* set multiple data out values using dedicate set/clear register */
-static void omap_set_gpio_dataout_reg_multiple(struct gpio_bank *bank,
-					       unsigned long *mask,
-					       unsigned long *bits)
-{
-	void __iomem *reg = bank->base;
-	u32 l;
-
-	l = *bits & *mask;
-	writel_relaxed(l, reg + bank->regs->set_dataout);
-	bank->context.dataout |= l;
-
-	l = ~*bits & *mask;
-	writel_relaxed(l, reg + bank->regs->clr_dataout);
-	bank->context.dataout &= ~l;
-}
-
-/* set multiple data out values using mask register */
-static void omap_set_gpio_dataout_mask_multiple(struct gpio_bank *bank,
-						unsigned long *mask,
-						unsigned long *bits)
-{
-	void __iomem *reg = bank->base + bank->regs->dataout;
-	u32 l = (readl_relaxed(reg) & ~*mask) | (*bits & *mask);
-
-	writel_relaxed(l, reg);
-	bank->context.dataout = l;
-}
-
 static inline void omap_gpio_rmw(void __iomem *base, u32 reg, u32 mask, bool set)
 {
 	int l = readl_relaxed(base + reg);
@@ -1037,10 +1006,14 @@ static void omap_gpio_set_multiple(struct gpio_chip *chip, unsigned long *mask,
 				   unsigned long *bits)
 {
 	struct gpio_bank *bank = gpiochip_get_data(chip);
+	void __iomem *reg = bank->base + bank->regs->dataout;
 	unsigned long flags;
+	u32 l;
 
 	raw_spin_lock_irqsave(&bank->lock, flags);
-	bank->set_dataout_multiple(bank, mask, bits);
+	l = (readl_relaxed(reg) & ~*mask) | (*bits & *mask);
+	writel_relaxed(l, reg);
+	bank->context.dataout = l;
 	raw_spin_unlock_irqrestore(&bank->lock, flags);
 }
 
@@ -1530,14 +1503,10 @@ static int omap_gpio_probe(struct platform_device *pdev)
 				pdata->get_context_loss_count;
 	}
 
-	if (bank->regs->set_dataout && bank->regs->clr_dataout) {
+	if (bank->regs->set_dataout && bank->regs->clr_dataout)
 		bank->set_dataout = omap_set_gpio_dataout_reg;
-		bank->set_dataout_multiple = omap_set_gpio_dataout_reg_multiple;
-	} else {
+	else
 		bank->set_dataout = omap_set_gpio_dataout_mask;
-		bank->set_dataout_multiple =
-				omap_set_gpio_dataout_mask_multiple;
-	}
 
 	raw_spin_lock_init(&bank->lock);
 	raw_spin_lock_init(&bank->wa_lock);
-- 
2.17.1

[PATCH-next 11/20] gpio: gpio-omap: simplify bank->level_mask

[Grygorii Strashko]

From: Russell King <rmk+kernel@armlinux.org.uk>

bank->level_mask is merely the bitwise or of the level detection
context which we have already read in this function.  Rather than
repeating additional reads, compute it from the values already
read.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/gpio/gpio-omap.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 8fdac6e4a929..369ce46e2b09 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -325,6 +325,9 @@ static inline void omap_set_gpio_trigger(struct gpio_bank *bank, int gpio,
 	bank->context.fallingdetect =
 			readl_relaxed(bank->base + bank->regs->fallingdetect);
 
+	bank->level_mask = bank->context.leveldetect0 |
+			   bank->context.leveldetect1;
+
 	if (likely(!(bank->non_wakeup_gpios & gpio_bit))) {
 		omap_gpio_rmw(base, bank->regs->wkup_en, gpio_bit, trigger != 0);
 		bank->context.wake_en =
@@ -344,10 +347,6 @@ static inline void omap_set_gpio_trigger(struct gpio_bank *bank, int gpio,
 		else
 			bank->enabled_non_wakeup_gpios &= ~gpio_bit;
 	}
-
-	bank->level_mask =
-		readl_relaxed(bank->base + bank->regs->leveldetect0) |
-		readl_relaxed(bank->base + bank->regs->leveldetect1);
 }
 
 #ifdef CONFIG_ARCH_OMAP1
-- 
2.17.1

[PATCH-next 12/20] gpio: gpio-omap: simplify read-modify-write

[Grygorii Strashko]

From: Russell King <rmk+kernel@armlinux.org.uk>

We already have a read-modify-write helper, but there's more that can
be done with a read-modify-write helper if it returned the new value.
Modify the existing helper to return the new value, and arrange for
it to take one less argument by having the caller compute the register
address.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/gpio/gpio-omap.c | 83 +++++++++++++++-------------------------
 1 file changed, 30 insertions(+), 53 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 369ce46e2b09..1a0890586b45 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -92,20 +92,25 @@ static inline struct gpio_bank *omap_irq_data_get_bank(struct irq_data *d)
 	return gpiochip_get_data(chip);
 }
 
-static void omap_set_gpio_direction(struct gpio_bank *bank, int gpio,
-				    int is_input)
+static inline u32 omap_gpio_rmw(void __iomem *reg, u32 mask, bool set)
 {
-	void __iomem *reg = bank->base;
-	u32 l;
+	u32 val = readl_relaxed(reg);
 
-	reg += bank->regs->direction;
-	l = readl_relaxed(reg);
-	if (is_input)
-		l |= BIT(gpio);
+	if (set)
+		val |= mask;
 	else
-		l &= ~(BIT(gpio));
-	writel_relaxed(l, reg);
-	bank->context.oe = l;
+		val &= ~mask;
+
+	writel_relaxed(val, reg);
+
+	return val;
+}
+
+static void omap_set_gpio_direction(struct gpio_bank *bank, int gpio,
+				    int is_input)
+{
+	bank->context.oe = omap_gpio_rmw(bank->base + bank->regs->direction,
+					 BIT(gpio), is_input);
 }
 
 
@@ -131,29 +136,8 @@ static void omap_set_gpio_dataout_reg(struct gpio_bank *bank, unsigned offset,
 static void omap_set_gpio_dataout_mask(struct gpio_bank *bank, unsigned offset,
 				       int enable)
 {
-	void __iomem *reg = bank->base + bank->regs->dataout;
-	u32 gpio_bit = BIT(offset);
-	u32 l;
-
-	l = readl_relaxed(reg);
-	if (enable)
-		l |= gpio_bit;
-	else
-		l &= ~gpio_bit;
-	writel_relaxed(l, reg);
-	bank->context.dataout = l;
-}
-
-static inline void omap_gpio_rmw(void __iomem *base, u32 reg, u32 mask, bool set)
-{
-	int l = readl_relaxed(base + reg);
-
-	if (set)
-		l |= mask;
-	else
-		l &= ~mask;
-
-	writel_relaxed(l, base + reg);
+	bank->context.dataout = omap_gpio_rmw(bank->base + bank->regs->dataout,
+					      BIT(offset), enable);
 }
 
 static inline void omap_gpio_dbck_enable(struct gpio_bank *bank)
@@ -217,16 +201,9 @@ static int omap2_set_gpio_debounce(struct gpio_bank *bank, unsigned offset,
 	reg = bank->base + bank->regs->debounce;
 	writel_relaxed(debounce, reg);
 
-	reg = bank->base + bank->regs->debounce_en;
-	val = readl_relaxed(reg);
-
-	if (enable)
-		val |= l;
-	else
-		val &= ~l;
+	val = omap_gpio_rmw(bank->base + bank->regs->debounce_en, l, enable);
 	bank->dbck_enable_mask = val;
 
-	writel_relaxed(val, reg);
 	clk_disable(bank->dbck);
 	/*
 	 * Enable debounce clock per module.
@@ -301,9 +278,9 @@ static inline void omap_set_gpio_trigger(struct gpio_bank *bank, int gpio,
 	void __iomem *base = bank->base;
 	u32 gpio_bit = BIT(gpio);
 
-	omap_gpio_rmw(base, bank->regs->leveldetect0, gpio_bit,
+	omap_gpio_rmw(base + bank->regs->leveldetect0, gpio_bit,
 		      trigger & IRQ_TYPE_LEVEL_LOW);
-	omap_gpio_rmw(base, bank->regs->leveldetect1, gpio_bit,
+	omap_gpio_rmw(base + bank->regs->leveldetect1, gpio_bit,
 		      trigger & IRQ_TYPE_LEVEL_HIGH);
 
 	/*
@@ -311,9 +288,9 @@ static inline void omap_set_gpio_trigger(struct gpio_bank *bank, int gpio,
 	 * to be woken from idle state.  Set the appropriate edge detection
 	 * in addition to the level detection.
 	 */
-	omap_gpio_rmw(base, bank->regs->risingdetect, gpio_bit,
+	omap_gpio_rmw(base + bank->regs->risingdetect, gpio_bit,
 		      trigger & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_LEVEL_HIGH));
-	omap_gpio_rmw(base, bank->regs->fallingdetect, gpio_bit,
+	omap_gpio_rmw(base + bank->regs->fallingdetect, gpio_bit,
 		      trigger & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_LEVEL_LOW));
 
 	bank->context.leveldetect0 =
@@ -329,7 +306,7 @@ static inline void omap_set_gpio_trigger(struct gpio_bank *bank, int gpio,
 			   bank->context.leveldetect1;
 
 	if (likely(!(bank->non_wakeup_gpios & gpio_bit))) {
-		omap_gpio_rmw(base, bank->regs->wkup_en, gpio_bit, trigger != 0);
+		omap_gpio_rmw(base + bank->regs->wkup_en, gpio_bit, trigger != 0);
 		bank->context.wake_en =
 			readl_relaxed(bank->base + bank->regs->wkup_en);
 	}
@@ -414,7 +391,7 @@ static int omap_set_gpio_triggering(struct gpio_bank *bank, int gpio,
 			l |= BIT(gpio << 1);
 
 		/* Enable wake-up during idle for dynamic tick */
-		omap_gpio_rmw(base, bank->regs->wkup_en, BIT(gpio), trigger);
+		omap_gpio_rmw(base + bank->regs->wkup_en, BIT(gpio), trigger);
 		bank->context.wake_en =
 			readl_relaxed(bank->base + bank->regs->wkup_en);
 		writel_relaxed(l, reg);
@@ -451,7 +428,7 @@ static void omap_disable_gpio_module(struct gpio_bank *bank, unsigned offset)
 	    !LINE_USED(bank->mod_usage, offset) &&
 	    !LINE_USED(bank->irq_usage, offset)) {
 		/* Disable wake-up during idle for dynamic tick */
-		omap_gpio_rmw(base, bank->regs->wkup_en, BIT(offset), 0);
+		omap_gpio_rmw(base + bank->regs->wkup_en, BIT(offset), 0);
 		bank->context.wake_en =
 			readl_relaxed(bank->base + bank->regs->wkup_en);
 	}
@@ -1046,9 +1023,9 @@ static void omap_gpio_mod_init(struct gpio_bank *bank)
 		return;
 	}
 
-	omap_gpio_rmw(base, bank->regs->irqenable, l,
+	omap_gpio_rmw(base + bank->regs->irqenable, l,
 		      bank->regs->irqenable_inv);
-	omap_gpio_rmw(base, bank->regs->irqstatus, l,
+	omap_gpio_rmw(base + bank->regs->irqstatus, l,
 		      !bank->regs->irqenable_inv);
 	if (bank->regs->debounce_en)
 		writel_relaxed(0, base + bank->regs->debounce_en);
@@ -1219,8 +1196,8 @@ static void omap_gpio_idle(struct gpio_bank *bank, bool may_lose_context)
 	 */
 	if (!bank->loses_context && bank->enabled_non_wakeup_gpios) {
 		nowake = bank->enabled_non_wakeup_gpios;
-		omap_gpio_rmw(base, bank->regs->fallingdetect, nowake, ~nowake);
-		omap_gpio_rmw(base, bank->regs->risingdetect, nowake, ~nowake);
+		omap_gpio_rmw(base + bank->regs->fallingdetect, nowake, ~nowake);
+		omap_gpio_rmw(base + bank->regs->risingdetect, nowake, ~nowake);
 	}
 
 update_gpio_context_count:
-- 
2.17.1

[PATCH-next 13/20] gpio: gpio-omap: simplify omap_toggle_gpio_edge_triggering()

[Grygorii Strashko]

From: Russell King <rmk+kernel@armlinux.org.uk>

This function open-codes an exclusive-or bitwise operation using an
if() statement and explicitly setting or clearing the bit. Instead,
use an exclusive-or operation instead, and simplify the function.

We can combine the preprocessor conditional using IS_ENABLED() and
gain some additional compilation coverage.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/gpio/gpio-omap.c | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 1a0890586b45..097ed8d1a117 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -326,32 +326,18 @@ static inline void omap_set_gpio_trigger(struct gpio_bank *bank, int gpio,
 	}
 }
 
-#ifdef CONFIG_ARCH_OMAP1
 /*
  * This only applies to chips that can't do both rising and falling edge
  * detection at once.  For all other chips, this function is a noop.
  */
 static void omap_toggle_gpio_edge_triggering(struct gpio_bank *bank, int gpio)
 {
-	void __iomem *reg = bank->base;
-	u32 l = 0;
-
-	if (!bank->regs->irqctrl)
-		return;
-
-	reg += bank->regs->irqctrl;
+	if (IS_ENABLED(CONFIG_ARCH_OMAP1) && bank->regs->irqctrl) {
+		void __iomem *reg = bank->base + bank->regs->irqctrl;
 
-	l = readl_relaxed(reg);
-	if ((l >> gpio) & 1)
-		l &= ~(BIT(gpio));
-	else
-		l |= BIT(gpio);
-
-	writel_relaxed(l, reg);
+		writel_relaxed(readl_relaxed(reg) ^ BIT(gpio), reg);
+	}
 }
-#else
-static void omap_toggle_gpio_edge_triggering(struct gpio_bank *bank, int gpio) {}
-#endif
 
 static int omap_set_gpio_triggering(struct gpio_bank *bank, int gpio,
 				    unsigned trigger)
-- 
2.17.1

[PATCH-next 14/20] gpio: gpio-omap: simplify omap_set_gpio_irqenable()

[Grygorii Strashko]

From: Russell King <rmk+kernel@armlinux.org.uk>

omap_set_gpio_irqenable() calls two helpers that are almost the same
apart from whether they set or clear bits. We can consolidate these:

- in the set/clear bit register case, we can perform the operation on
  our saved context copy and write the appropriate set/clear register.
- otherwise, we can use our read-modify-write helper and invert enable
  if irqenable_inv is set.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/gpio/gpio-omap.c | 61 ++++++++++------------------------------
 1 file changed, 15 insertions(+), 46 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 097ed8d1a117..a90e27d7ce5e 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -529,57 +529,26 @@ static u32 omap_get_gpio_irqbank_mask(struct gpio_bank *bank)
 	return l;
 }
 
-static void omap_enable_gpio_irqbank(struct gpio_bank *bank, int gpio_mask)
-{
-	void __iomem *reg = bank->base;
-	u32 l;
-
-	if (bank->regs->set_irqenable) {
-		reg += bank->regs->set_irqenable;
-		l = gpio_mask;
-		bank->context.irqenable1 |= gpio_mask;
-	} else {
-		reg += bank->regs->irqenable;
-		l = readl_relaxed(reg);
-		if (bank->regs->irqenable_inv)
-			l &= ~gpio_mask;
-		else
-			l |= gpio_mask;
-		bank->context.irqenable1 = l;
-	}
-
-	writel_relaxed(l, reg);
-}
-
-static void omap_disable_gpio_irqbank(struct gpio_bank *bank, int gpio_mask)
+static inline void omap_set_gpio_irqenable(struct gpio_bank *bank,
+					   unsigned offset, int enable)
 {
 	void __iomem *reg = bank->base;
-	u32 l;
+	u32 gpio_mask = BIT(offset);
 
-	if (bank->regs->clr_irqenable) {
-		reg += bank->regs->clr_irqenable;
-		l = gpio_mask;
-		bank->context.irqenable1 &= ~gpio_mask;
+	if (bank->regs->set_irqenable && bank->regs->clr_irqenable) {
+		if (enable) {
+			reg += bank->regs->set_irqenable;
+			bank->context.irqenable1 |= gpio_mask;
+		} else {
+			reg += bank->regs->clr_irqenable;
+			bank->context.irqenable1 &= ~gpio_mask;
+		}
+		writel_relaxed(gpio_mask, reg);
 	} else {
-		reg += bank->regs->irqenable;
-		l = readl_relaxed(reg);
-		if (bank->regs->irqenable_inv)
-			l |= gpio_mask;
-		else
-			l &= ~gpio_mask;
-		bank->context.irqenable1 = l;
+		bank->context.irqenable1 =
+			omap_gpio_rmw(reg + bank->regs->irqenable, gpio_mask,
+				      enable ^ bank->regs->irqenable_inv);
 	}
-
-	writel_relaxed(l, reg);
-}
-
-static inline void omap_set_gpio_irqenable(struct gpio_bank *bank,
-					   unsigned offset, int enable)
-{
-	if (enable)
-		omap_enable_gpio_irqbank(bank, BIT(offset));
-	else
-		omap_disable_gpio_irqbank(bank, BIT(offset));
 }
 
 /* Use disable_irq_wake() and enable_irq_wake() functions from drivers */
-- 
2.17.1

[PATCH-next 15/20] gpio: gpio-omap: remove dataout variation in context handling

[Grygorii Strashko]

From: Russell King <rmk+kernel@armlinux.org.uk>

When a GPIO block has the set/clear dataout registers implemented, it
also has the normal dataout register implemented. Reading this register
reads the current GPIO output state, and writing it sets the GPIOs to
the explicit state. This is the behaviour that we want when saving and
restoring the context, so use the dataout register exclusively.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/gpio/gpio-omap.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index a90e27d7ce5e..c24a91b2df97 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1087,11 +1087,7 @@ static void omap_gpio_init_context(struct gpio_bank *p)
 	p->context.fallingdetect = readl_relaxed(base + regs->fallingdetect);
 	p->context.irqenable1	= readl_relaxed(base + regs->irqenable);
 	p->context.irqenable2	= readl_relaxed(base + regs->irqenable2);
-
-	if (regs->set_dataout && p->regs->clr_dataout)
-		p->context.dataout = readl_relaxed(base + regs->set_dataout);
-	else
-		p->context.dataout = readl_relaxed(base + regs->dataout);
+	p->context.dataout	= readl_relaxed(base + regs->dataout);
 
 	p->context_valid = true;
 }
@@ -1109,11 +1105,7 @@ static void omap_gpio_restore_context(struct gpio_bank *bank)
 				bank->base + bank->regs->risingdetect);
 	writel_relaxed(bank->context.fallingdetect,
 				bank->base + bank->regs->fallingdetect);
-	if (bank->regs->set_dataout && bank->regs->clr_dataout)
-		writel_relaxed(bank->context.dataout,
-				bank->base + bank->regs->set_dataout);
-	else
-		writel_relaxed(bank->context.dataout,
+	writel_relaxed(bank->context.dataout,
 				bank->base + bank->regs->dataout);
 	writel_relaxed(bank->context.oe, bank->base + bank->regs->direction);
 
-- 
2.17.1

[PATCH-next 16/20] gpio: gpio-omap: clean up omap_gpio_restore_context()

[Grygorii Strashko]

From: Russell King <rmk+kernel@armlinux.org.uk>

Use local variables to store the base iomem address and regs table
pointer like omap_gpio_init_context() does. Not only does this make
the function neater, it also avoids unnecessary reloads of the same
data multiple times.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/gpio/gpio-omap.c | 36 +++++++++++++++---------------------
 1 file changed, 15 insertions(+), 21 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index c24a91b2df97..1c5fa12bcf9f 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1094,32 +1094,26 @@ static void omap_gpio_init_context(struct gpio_bank *p)
 
 static void omap_gpio_restore_context(struct gpio_bank *bank)
 {
-	writel_relaxed(bank->context.wake_en,
-				bank->base + bank->regs->wkup_en);
-	writel_relaxed(bank->context.ctrl, bank->base + bank->regs->ctrl);
-	writel_relaxed(bank->context.leveldetect0,
-				bank->base + bank->regs->leveldetect0);
-	writel_relaxed(bank->context.leveldetect1,
-				bank->base + bank->regs->leveldetect1);
-	writel_relaxed(bank->context.risingdetect,
-				bank->base + bank->regs->risingdetect);
-	writel_relaxed(bank->context.fallingdetect,
-				bank->base + bank->regs->fallingdetect);
-	writel_relaxed(bank->context.dataout,
-				bank->base + bank->regs->dataout);
-	writel_relaxed(bank->context.oe, bank->base + bank->regs->direction);
+	struct omap_gpio_reg_offs *regs = bank->regs;
+	void __iomem *base = bank->base;
+
+	writel_relaxed(bank->context.wake_en, base + regs->wkup_en);
+	writel_relaxed(bank->context.ctrl, base + regs->ctrl);
+	writel_relaxed(bank->context.leveldetect0, base + regs->leveldetect0);
+	writel_relaxed(bank->context.leveldetect1, base + regs->leveldetect1);
+	writel_relaxed(bank->context.risingdetect, base + regs->risingdetect);
+	writel_relaxed(bank->context.fallingdetect, base + regs->fallingdetect);
+	writel_relaxed(bank->context.dataout, base + regs->dataout);
+	writel_relaxed(bank->context.oe, base + regs->direction);
 
 	if (bank->dbck_enable_mask) {
-		writel_relaxed(bank->context.debounce, bank->base +
-					bank->regs->debounce);
+		writel_relaxed(bank->context.debounce, base + regs->debounce);
 		writel_relaxed(bank->context.debounce_en,
-					bank->base + bank->regs->debounce_en);
+			       base + regs->debounce_en);
 	}
 
-	writel_relaxed(bank->context.irqenable1,
-				bank->base + bank->regs->irqenable);
-	writel_relaxed(bank->context.irqenable2,
-				bank->base + bank->regs->irqenable2);
+	writel_relaxed(bank->context.irqenable1, base + regs->irqenable);
+	writel_relaxed(bank->context.irqenable2, base + regs->irqenable2);
 }
 
 static void omap_gpio_idle(struct gpio_bank *bank, bool may_lose_context)
-- 
2.17.1

[PATCH-next 17/20] gpio: gpio-omap: constify register tables

[Grygorii Strashko]

From: Russell King <rmk+kernel@armlinux.org.uk>

We must never alter the register tables; these are read-only as far
as the driver is concerned.  Constify these tables.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/gpio/gpio-omap.c                | 12 ++++++------
 include/linux/platform_data/gpio-omap.h |  2 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 1c5fa12bcf9f..039bbb1ae6cb 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -48,6 +48,8 @@ struct gpio_regs {
 
 struct gpio_bank {
 	void __iomem *base;
+	const struct omap_gpio_reg_offs *regs;
+
 	int irq;
 	u32 non_wakeup_gpios;
 	u32 enabled_non_wakeup_gpios;
@@ -75,8 +77,6 @@ struct gpio_bank {
 
 	void (*set_dataout)(struct gpio_bank *bank, unsigned gpio, int enable);
 	int (*get_context_loss_count)(struct device *dev);
-
-	struct omap_gpio_reg_offs *regs;
 };
 
 #define GPIO_MOD_CTRL_BIT	BIT(0)
@@ -1075,7 +1075,7 @@ static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc)
 
 static void omap_gpio_init_context(struct gpio_bank *p)
 {
-	struct omap_gpio_reg_offs *regs = p->regs;
+	const struct omap_gpio_reg_offs *regs = p->regs;
 	void __iomem *base = p->base;
 
 	p->context.ctrl		= readl_relaxed(base + regs->ctrl);
@@ -1094,7 +1094,7 @@ static void omap_gpio_init_context(struct gpio_bank *p)
 
 static void omap_gpio_restore_context(struct gpio_bank *bank)
 {
-	struct omap_gpio_reg_offs *regs = bank->regs;
+	const struct omap_gpio_reg_offs *regs = bank->regs;
 	void __iomem *base = bank->base;
 
 	writel_relaxed(bank->context.wake_en, base + regs->wkup_en);
@@ -1267,7 +1267,7 @@ static int gpio_omap_cpu_notifier(struct notifier_block *nb,
 	return NOTIFY_OK;
 }
 
-static struct omap_gpio_reg_offs omap2_gpio_regs = {
+static const struct omap_gpio_reg_offs omap2_gpio_regs = {
 	.revision =		OMAP24XX_GPIO_REVISION,
 	.direction =		OMAP24XX_GPIO_OE,
 	.datain =		OMAP24XX_GPIO_DATAIN,
@@ -1290,7 +1290,7 @@ static struct omap_gpio_reg_offs omap2_gpio_regs = {
 	.fallingdetect =	OMAP24XX_GPIO_FALLINGDETECT,
 };
 
-static struct omap_gpio_reg_offs omap4_gpio_regs = {
+static const struct omap_gpio_reg_offs omap4_gpio_regs = {
 	.revision =		OMAP4_GPIO_REVISION,
 	.direction =		OMAP4_GPIO_OE,
 	.datain =		OMAP4_GPIO_DATAIN,
diff --git a/include/linux/platform_data/gpio-omap.h b/include/linux/platform_data/gpio-omap.h
index 17edc43201d2..8b30b14b47d3 100644
--- a/include/linux/platform_data/gpio-omap.h
+++ b/include/linux/platform_data/gpio-omap.h
@@ -186,7 +186,7 @@ struct omap_gpio_platform_data {
 	bool is_mpuio;		/* whether the bank is of type MPUIO */
 	u32 non_wakeup_gpios;
 
-	struct omap_gpio_reg_offs *regs;
+	const struct omap_gpio_reg_offs *regs;
 
 	/* Return context loss count due to PM states changing */
 	int (*get_context_loss_count)(struct device *dev);
-- 
2.17.1

[PATCH-next 18/20] gpio: gpio-omap: clean up wakeup handling

[Grygorii Strashko]

From: Russell King <rmk+kernel@armlinux.org.uk>

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/gpio/gpio-omap.c | 36 +++++++++++++-----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 039bbb1ae6cb..61e32c7e6670 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -305,12 +305,6 @@ static inline void omap_set_gpio_trigger(struct gpio_bank *bank, int gpio,
 	bank->level_mask = bank->context.leveldetect0 |
 			   bank->context.leveldetect1;
 
-	if (likely(!(bank->non_wakeup_gpios & gpio_bit))) {
-		omap_gpio_rmw(base + bank->regs->wkup_en, gpio_bit, trigger != 0);
-		bank->context.wake_en =
-			readl_relaxed(bank->base + bank->regs->wkup_en);
-	}
-
 	/* This part needs to be executed always for OMAP{34xx, 44xx} */
 	if (!bank->regs->irqctrl && !omap_gpio_is_off_wakeup_capable(bank, gpio)) {
 		/*
@@ -343,7 +337,6 @@ static int omap_set_gpio_triggering(struct gpio_bank *bank, int gpio,
 				    unsigned trigger)
 {
 	void __iomem *reg = bank->base;
-	void __iomem *base = bank->base;
 	u32 l = 0;
 
 	if (bank->regs->leveldetect0 && bank->regs->wkup_en) {
@@ -375,11 +368,6 @@ static int omap_set_gpio_triggering(struct gpio_bank *bank, int gpio,
 			l |= 2 << (gpio << 1);
 		if (trigger & IRQ_TYPE_EDGE_FALLING)
 			l |= BIT(gpio << 1);
-
-		/* Enable wake-up during idle for dynamic tick */
-		omap_gpio_rmw(base + bank->regs->wkup_en, BIT(gpio), trigger);
-		bank->context.wake_en =
-			readl_relaxed(bank->base + bank->regs->wkup_en);
 		writel_relaxed(l, reg);
 	}
 	return 0;
@@ -408,17 +396,6 @@ static void omap_enable_gpio_module(struct gpio_bank *bank, unsigned offset)
 
 static void omap_disable_gpio_module(struct gpio_bank *bank, unsigned offset)
 {
-	void __iomem *base = bank->base;
-
-	if (bank->regs->wkup_en &&
-	    !LINE_USED(bank->mod_usage, offset) &&
-	    !LINE_USED(bank->irq_usage, offset)) {
-		/* Disable wake-up during idle for dynamic tick */
-		omap_gpio_rmw(base + bank->regs->wkup_en, BIT(offset), 0);
-		bank->context.wake_en =
-			readl_relaxed(bank->base + bank->regs->wkup_en);
-	}
-
 	if (bank->regs->ctrl && !BANK_USED(bank)) {
 		void __iomem *reg = bank->base + bank->regs->ctrl;
 		u32 ctrl;
@@ -549,6 +526,19 @@ static inline void omap_set_gpio_irqenable(struct gpio_bank *bank,
 			omap_gpio_rmw(reg + bank->regs->irqenable, gpio_mask,
 				      enable ^ bank->regs->irqenable_inv);
 	}
+
+	/*
+	 * Program GPIO wakeup along with IRQ enable to satisfy OMAP4430 TRM
+	 * note requiring correlation between the IRQ enable registers and
+	 * the wakeup registers.  In any case, we want wakeup from idle
+	 * enabled for the GPIOs which support this feature.
+	 */
+	if (bank->regs->wkup_en &&
+	    (bank->regs->edgectrl1 || !(bank->non_wakeup_gpios & gpio_mask))) {
+		bank->context.wake_en =
+			omap_gpio_rmw(bank->base + bank->regs->wkup_en,
+				      gpio_mask, enable);
+	}
 }
 
 /* Use disable_irq_wake() and enable_irq_wake() functions from drivers */
-- 
2.17.1

[PATCH-next 19/20] gpio: gpio-omap: irq_startup() must not return error codes

[Grygorii Strashko]

From: Russell King <rmk+kernel@armlinux.org.uk>

The irq_startup() method returns an unsigned int, but in __irq_startup()
it is assigned to an int.  However, nothing checks for errors, so any
error that is returned is ignored.

Remove the check for GPIO-input mode and the error return.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/gpio/gpio-omap.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 61e32c7e6670..94b10dfbff3f 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -635,8 +635,6 @@ static unsigned int omap_gpio_irq_startup(struct irq_data *d)
 
 	if (!LINE_USED(bank->mod_usage, offset))
 		omap_set_gpio_direction(bank, offset, 1);
-	else if (!omap_gpio_is_input(bank, offset))
-		goto err;
 	omap_enable_gpio_module(bank, offset);
 	bank->irq_usage |= BIT(offset);
 
@@ -644,9 +642,6 @@ static unsigned int omap_gpio_irq_startup(struct irq_data *d)
 	omap_gpio_unmask_irq(d);
 
 	return 0;
-err:
-	raw_spin_unlock_irqrestore(&bank->lock, flags);
-	return -EINVAL;
 }
 
 static void omap_gpio_irq_shutdown(struct irq_data *d)
-- 
2.17.1

[PATCH-next 20/20] gpio: gpio-omap: clean up register access in omap2_set_gpio_debounce()

[Grygorii Strashko]

From: Russell King <rmk+kernel@armlinux.org.uk>

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/gpio/gpio-omap.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 94b10dfbff3f..52592eac0895 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -181,7 +181,6 @@ static inline void omap_gpio_dbck_disable(struct gpio_bank *bank)
 static int omap2_set_gpio_debounce(struct gpio_bank *bank, unsigned offset,
 				   unsigned debounce)
 {
-	void __iomem		*reg;
 	u32			val;
 	u32			l;
 	bool			enable = !!debounce;
@@ -198,8 +197,7 @@ static int omap2_set_gpio_debounce(struct gpio_bank *bank, unsigned offset,
 	l = BIT(offset);
 
 	clk_enable(bank->dbck);
-	reg = bank->base + bank->regs->debounce;
-	writel_relaxed(debounce, reg);
+	writel_relaxed(debounce, bank->base + bank->regs->debounce);
 
 	val = omap_gpio_rmw(bank->base + bank->regs->debounce_en, l, enable);
 	bank->dbck_enable_mask = val;
-- 
2.17.1

Re: [PATCH-next 00/20] gpio: gpio-omap: set of fixes and big clean-up

[Tony Lindgren]

Hi,

* Grygorii Strashko <grygorii.strashko@ti.com> [190610 10:11]:
> Hi Linus, Russell, Tony, All,
> 
> This series contains set of patches from Russell King which were circulated
> internally for quite some time already and I fill it's reasonable to move
> future discussion upstream (and also avoid rebasing).
> Fisrt two patches are fixes and the rest are big, great clean up
> from Russell King.
> 
> Personally, I like this clean up and refactoring very much and don't want
> it to be lost.

Adding Aaro to Cc too hopefully for more testing. Yes this is very nice
and behaves for my idle test cases. I've also boot tested omap1 osk and
it still works just fine for NFSroot.

FYI, after this series, the only issue I'm aware of still remaining
is the lost edge wake-up interrupts for L4 PER idle that I posted a WIP
fix as "[PATCH] gpio: gpio-omap: Fix lost edge wake-up interrupts".
But that one still needs a bit more work and is a separate fix from this
series.

So for this whole series, please feel free to add:

Tested-by: Tony Lindgren <tony@atomide.com>



> Code can be found at:
>  git@git.ti.com:~gragst/ti-linux-kernel/gragsts-ti-linux-kernel.git
> branch:
>  lkml-next-gpio-clean-up
> 
> Russell King (20):
>   gpio: gpio-omap: ensure irq is enabled before wakeup
>   gpio: gpio-omap: fix lack of irqstatus_raw0 for OMAP4
>   gpio: gpio-omap: remove remainder of list management
>   gpio: gpio-omap: clean up edge interrupt handling
>   gpio: gpio-omap: remove irq_ack method
>   gpio: gpio-omap: move omap_gpio_request() and omap_gpio_free()
>   gpio: gpio-omap: simplify omap_gpio_get_direction()
>   gpio: gpio-omap: simplify get() method
>   gpio: gpio-omap: simplify get_multiple()
>   gpio: gpio-omap: simplify set_multiple()
>   gpio: gpio-omap: simplify bank->level_mask
>   gpio: gpio-omap: simplify read-modify-write
>   gpio: gpio-omap: simplify omap_toggle_gpio_edge_triggering()
>   gpio: gpio-omap: simplify omap_set_gpio_irqenable()
>   gpio: gpio-omap: remove dataout variation in context handling
>   gpio: gpio-omap: clean up omap_gpio_restore_context()
>   gpio: gpio-omap: constify register tables
>   gpio: gpio-omap: clean up wakeup handling
>   gpio: gpio-omap: irq_startup() must not return error codes
>   gpio: gpio-omap: clean up register access in omap2_set_gpio_debounce()
> 
>  drivers/gpio/gpio-omap.c                | 497 ++++++++----------------
>  include/linux/platform_data/gpio-omap.h |   2 +-
>  2 files changed, 161 insertions(+), 338 deletions(-)
> 
> -- 
> 2.17.1
> 

Re: [PATCH-next 01/20] gpio: gpio-omap: ensure irq is enabled before wakeup

[Linus Walleij]

On Mon, Jun 10, 2019 at 7:11 PM Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> From: Russell King <rmk+kernel@armlinux.org.uk>
>
> Documentation states:
>
>   NOTE: There must be a correlation between the wake-up enable and
>   interrupt-enable registers. If a GPIO pin has a wake-up configured
>   on it, it must also have the corresponding interrupt enabled (on
>   one of the two interrupt lines).
>
> Ensure that this condition is always satisfied by enabling the detection
> events after enabling the interrupt, and disabling the detection before
> disabling the interrupt.  This ensures interrupt/wakeup events can not
> happen until both the wakeup and interrupt enables correlate.
>
> If we do any clearing, clear between the interrupt enable/disable and
> trigger setting.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH-next 02/20] gpio: gpio-omap: fix lack of irqstatus_raw0 for OMAP4

[Linus Walleij]

On Mon, Jun 10, 2019 at 7:11 PM Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> From: Russell King <rmk+kernel@armlinux.org.uk>
>
> Commit 384ebe1c2849 ("gpio/omap: Add DT support to GPIO driver") added
> the register definition tables to the gpio-omap driver. Subsequently to
> that commit, commit 4e962e8998cc ("gpio/omap: remove cpu_is_omapxxxx()
> checks from *_runtime_resume()") added definitions for irqstatus_raw*
> registers to the legacy OMAP4 definitions, but missed the DT
> definitions.
>
> This causes an unintentional change of behaviour for the 1.101 errata
> workaround on OMAP4 platforms. Fix this oversight.
>
> Fixes: 4e962e8998cc ("gpio/omap: remove cpu_is_omapxxxx() checks from *_runtime_resume()")
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH-next 03/20] gpio: gpio-omap: remove remainder of list management

[Linus Walleij]

On Mon, Jun 10, 2019 at 7:11 PM Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> From: Russell King <rmk+kernel@armlinux.org.uk>
>
> Commit c4791bc6e3a6 ("gpio: omap: drop omap_gpio_list") removed the
> list head and addition to the list head of each gpio bank, but failed
> to remove the list_del() call and the node inside struct gpio_bank.
> Remove these too.
>
> Fixes: c4791bc6e3a6 ("gpio: omap: drop omap_gpio_list")
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH-next 04/20] gpio: gpio-omap: clean up edge interrupt handling

[Linus Walleij]

On Mon, Jun 10, 2019 at 7:11 PM Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> From: Russell King <rmk+kernel@armlinux.org.uk>
>
> The edge interrupt handling was effectively:
>
>         isr = ISR_reg & enabled;
>         if (bank->level_mask)
>                 level_mask = bank->level_mask & enabled;
>         else
>                 level_mask = 0;
>
>         edge = isr & ~level_mask;
>
> When bank->level_mask is zero, level_mask will be computed as zero
> anyway, so the if() statement is redundant.  We are then left with:
>
>         isr = ISR_reg & enabled;
>         level_mask = bank->level_mask & enabled;
>         edge = isr & ~level_mask;
>
> This can be simplified further to:
>
>         isr = ISR_reg & enabled;
>         edge = isr & ~bank->level_mask;
>
> since the second mask with 'enabled' is redundant.
>
> Improve the associated comment as well.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH-next 05/20] gpio: gpio-omap: remove irq_ack method

[Linus Walleij]

On Mon, Jun 10, 2019 at 7:11 PM Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> From: Russell King <rmk+kernel@armlinux.org.uk>
>
> The irq_ack method does not fit our hardware requirements. Edge
> interrupts must be cleared before we handle them, and level interrupts
> must be cleared after handling them.
>
> We handle the interrupt clearance in our interrupt handler for edge IRQs
> and in the unmask method for level IRQs.
>
> Replace the irq_ack method with the no-op method from the dummy irq
> chip.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH-next 06/20] gpio: gpio-omap: move omap_gpio_request() and omap_gpio_free()

[Linus Walleij]

On Mon, Jun 10, 2019 at 7:12 PM Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> From: Russell King <rmk+kernel@armlinux.org.uk>
>
> Move these two functions to live beside the rest of the gpio chip
> implementation, rather than in the middle of the irq chip
> implementation.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH-next 07/20] gpio: gpio-omap: simplify omap_gpio_get_direction()

[Linus Walleij]

On Mon, Jun 10, 2019 at 7:12 PM Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> From: Russell King <rmk+kernel@armlinux.org.uk>
>
> Architectures are single-copy atomic, which means that simply reading
> a register is an inherently atomic operation.  There is no need to
> take a spinlock here.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

Patch applied.

This makes me wonder how many more instances we have
of this kind of mistake in the kernel :/ I guess it is mostly
harmless but it sure makes for massive code complexity.

I would sure like e.g. drivers/gpio/gpio-mmio.c to avoid
taking locks on architectures where this is not a problem
given it is used on a plethora of architectures.

Yours,
Linus Walleij

Re: [PATCH-next 08/20] gpio: gpio-omap: simplify get() method

[Linus Walleij]

On Mon, Jun 10, 2019 at 7:12 PM Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> From: Russell King <rmk+kernel@armlinux.org.uk>
>
> omap_gpio_get() calls omap_get_gpio_datain() or omap_get_gpio_dataout()
> to read the GPIO state. These two functions are only called from this
> method, so they don't add much value.  Move their contents into
> omap_gpio_get() method and simplify.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH-next 09/20] gpio: gpio-omap: simplify get_multiple()

[Linus Walleij]

On Mon, Jun 10, 2019 at 7:12 PM Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> From: Russell King <rmk+kernel@armlinux.org.uk>
>
> There is no reason to have helper functions to read the datain and
> dataout registers when they are only used in one location.  Simplify
> this code to make it more readable.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH-next 10/20] gpio: gpio-omap: simplify set_multiple()

[Linus Walleij]

On Mon, Jun 10, 2019 at 7:12 PM Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> From: Russell King <rmk+kernel@armlinux.org.uk>
>
> One of the reasons for set_multiple() to exist is to allow multiple
> GPIOs on the same chip to be changed simultaneously - see commit
> 5f42424354f5 ("gpiolib: allow simultaneous setting of multiple GPIO
> outputs"):
>
>  - Simultaneous glitch-free setting of multiple pins on any kind of
>    parallel bus attached to GPIOs provided they all reside on the
>    same chip and bank.
>
> In order for this to work, we should not use the atomic set/clear
> registers, but instead read-modify-write the dataout register.  We
> already take the spinlock to ensure that happens atomically, so
> move the code into the set_multiple() function and kill the two
> helper functions.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH-next 12/20] gpio: gpio-omap: simplify read-modify-write

[Linus Walleij]

On Mon, Jun 10, 2019 at 7:12 PM Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> From: Russell King <rmk+kernel@armlinux.org.uk>
>
> We already have a read-modify-write helper, but there's more that can
> be done with a read-modify-write helper if it returned the new value.
> Modify the existing helper to return the new value, and arrange for
> it to take one less argument by having the caller compute the register
> address.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH-next 13/20] gpio: gpio-omap: simplify omap_toggle_gpio_edge_triggering()

[Linus Walleij]

On Mon, Jun 10, 2019 at 7:12 PM Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> From: Russell King <rmk+kernel@armlinux.org.uk>
>
> This function open-codes an exclusive-or bitwise operation using an
> if() statement and explicitly setting or clearing the bit. Instead,
> use an exclusive-or operation instead, and simplify the function.
>
> We can combine the preprocessor conditional using IS_ENABLED() and
> gain some additional compilation coverage.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH-next 14/20] gpio: gpio-omap: simplify omap_set_gpio_irqenable()

[Linus Walleij]

On Mon, Jun 10, 2019 at 7:12 PM Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> From: Russell King <rmk+kernel@armlinux.org.uk>
>
> omap_set_gpio_irqenable() calls two helpers that are almost the same
> apart from whether they set or clear bits. We can consolidate these:
>
> - in the set/clear bit register case, we can perform the operation on
>   our saved context copy and write the appropriate set/clear register.
> - otherwise, we can use our read-modify-write helper and invert enable
>   if irqenable_inv is set.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH-next 15/20] gpio: gpio-omap: remove dataout variation in context handling

[Linus Walleij]

On Mon, Jun 10, 2019 at 7:13 PM Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> From: Russell King <rmk+kernel@armlinux.org.uk>
>
> When a GPIO block has the set/clear dataout registers implemented, it
> also has the normal dataout register implemented. Reading this register
> reads the current GPIO output state, and writing it sets the GPIOs to
> the explicit state. This is the behaviour that we want when saving and
> restoring the context, so use the dataout register exclusively.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH-next 16/20] gpio: gpio-omap: clean up omap_gpio_restore_context()

[Linus Walleij]

On Mon, Jun 10, 2019 at 7:13 PM Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> From: Russell King <rmk+kernel@armlinux.org.uk>
>
> Use local variables to store the base iomem address and regs table
> pointer like omap_gpio_init_context() does. Not only does this make
> the function neater, it also avoids unnecessary reloads of the same
> data multiple times.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH-next 17/20] gpio: gpio-omap: constify register tables

[Linus Walleij]

On Mon, Jun 10, 2019 at 7:13 PM Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> From: Russell King <rmk+kernel@armlinux.org.uk>
>
> We must never alter the register tables; these are read-only as far
> as the driver is concerned.  Constify these tables.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH-next 18/20] gpio: gpio-omap: clean up wakeup handling

[Linus Walleij]

On Mon, Jun 10, 2019 at 7:13 PM Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> From: Russell King <rmk+kernel@armlinux.org.uk>
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH-next 19/20] gpio: gpio-omap: irq_startup() must not return error codes

[Linus Walleij]

On Mon, Jun 10, 2019 at 7:13 PM Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> From: Russell King <rmk+kernel@armlinux.org.uk>
>
> The irq_startup() method returns an unsigned int, but in __irq_startup()
> it is assigned to an int.  However, nothing checks for errors, so any
> error that is returned is ignored.
>
> Remove the check for GPIO-input mode and the error return.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH-next 20/20] gpio: gpio-omap: clean up register access in omap2_set_gpio_debounce()

[Linus Walleij]

On Mon, Jun 10, 2019 at 7:13 PM Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> From: Russell King <rmk+kernel@armlinux.org.uk>
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

Patch applied.

At your convenience please look at the debounce function a bit
closer because I think there is a bug/unpredictable behavior:

IIUC the whole bank/block of GPIOs share the same debounce
timer setting, and it is currently handled in a "last caller wins"
manner, so if the different GPIOs in the bank has different
debounce settings, the call order decides what debounce time
is used across all of them.

In drivers/gpio/gpio-ftgpio.c function ftgpio_gpio_set_config()
I simply reject a dounce time setting
different from the currently configured if any GPIOs are
currently using the deounce feature.

(It's the semantic I came up with but maybe there are other
ideas here.)

Yours,
Linus Walleij

Re: [PATCH-next 00/20] gpio: gpio-omap: set of fixes and big clean-up

[Linus Walleij]

On Mon, Jun 10, 2019 at 7:11 PM Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> This series contains set of patches from Russell King which were circulated
> internally for quite some time already and I fill it's reasonable to move
> future discussion upstream (and also avoid rebasing).
> Fisrt two patches are fixes and the rest are big, great clean up
> from Russell King.
>
> Personally, I like this clean up and refactoring very much and don't want
> it to be lost.

I share your view, it is very nice to have Russell's attention to detail
shaping up this driver.

I vaguely remember at some point wondering why we were
not using gpio-mmio.c at least partially
for this driver, as it share this characteristic of keeping a shadow
copy of the registers around and seem to have offsets from 0..n
in the registers, but I guess there is some specific
good reason for not using the library?

Yours,
Linus Walleij

Re: [PATCH-next 20/20] gpio: gpio-omap: clean up register access in omap2_set_gpio_debounce()

[grygorii]

On 12/06/2019 12:11, Linus Walleij wrote:
> On Mon, Jun 10, 2019 at 7:13 PM Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
> 
>> From: Russell King <rmk+kernel@armlinux.org.uk>
>>
>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> 
> Patch applied.

Thanks Linus.

> 
> At your convenience please look at the debounce function a bit
> closer because I think there is a bug/unpredictable behavior:
> 
> IIUC the whole bank/block of GPIOs share the same debounce
> timer setting, and it is currently handled in a "last caller wins"
> manner, so if the different GPIOs in the bank has different
> debounce settings, the call order decides what debounce time
> is used across all of them.

Yeah. this is a "known" problem :(

> 
> In drivers/gpio/gpio-ftgpio.c function ftgpio_gpio_set_config()
> I simply reject a dounce time setting
> different from the currently configured if any GPIOs are
> currently using the deounce feature.
> 
> (It's the semantic I came up with but maybe there are other
> ideas here.)


There is one more patch in int. queue for debounce, but I've
decided to send it after this series as OMAP debounce changes are
usually debatable.

I'll look at gpio-ftgpio also.

By the way, there seems no ABI for debounce cfg in gpio_dev interface.
(or i'm missing smth?)

-- 
Best regards,
grygorii

Re: [PATCH-next 20/20] gpio: gpio-omap: clean up register access in omap2_set_gpio_debounce()

[Linus Walleij]

On Mon, Jun 17, 2019 at 8:57 PM grygorii <grygorii.strashko@ti.com> wrote:

> By the way, there seems no ABI for debounce cfg in gpio_dev interface.
> (or i'm missing smth?)

No that is correct.

I tend to be conservative with exposing stuff to userspace, but if somone
comes with a valid usecase for using debounce from userspace we will
add it I suppose. It's pretty much on need-to-have basis since we can
never change the userspace ABI.

Yours,
Linus Walleij