RE: [PATCH] gpio: mmio: Support two direction registers

[Shravan Ramani <sramani@xxxxxxxxxxxx>]

Thank you for the patch. I have tested it by using bgpio_init() in my driver and it works well.

Regards,
Shravan

-----Original Message-----
From: Linus Walleij <linus.walleij@xxxxxxxxxx> 
Sent: Friday, February 22, 2019 5:22 AM
To: linux-gpio@xxxxxxxxxxxxxxx
Cc: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>; Linus Walleij <linus.walleij@xxxxxxxxxx>; David Woods <dwoods@xxxxxxxxxxxx>; Shravan Ramani <sramani@xxxxxxxxxxxx>
Subject: [PATCH] gpio: mmio: Support two direction registers

It turns out that one specific hardware has two direction
registers: one to set a GPIO line as input and another one to set a GPIO line as output. So in theory a line can be configured as input and output at the same time.

Make the MMIO GPIO helper deal with this: store both registers in the state container, use both in the generic code if present. Synchronize the input register to the output register when we register a GPIO chip, with the output settings taking precedence.

Keep the helper variable to detect inverted direction semantics (only direction in register) but augment the code to be more straight-forward for the generic case when setting the registers.

Fix some flunky with unreadable direction registers at the same time as we're touching this code.

Cc: David Woods <dwoods@xxxxxxxxxxxx>
Cc: Shravan Kumar Ramani <sramani@xxxxxxxxxxxx>
Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
---
David/Shravan: plese test if this can be used to cut down the code in your driver to just use GPIO_GENERIC and bgpio_init().
---
 drivers/gpio/gpio-mmio.c    | 85 ++++++++++++++++++++++++-------------
 include/linux/gpio/driver.h | 15 +++++--
 2 files changed, 67 insertions(+), 33 deletions(-)

diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c index 50bdc29591c0..f172b4382d8d 100644
--- a/drivers/gpio/gpio-mmio.c
+++ b/drivers/gpio/gpio-mmio.c
@@ -372,11 +372,12 @@ static int bgpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
 
 	spin_lock_irqsave(&gc->bgpio_lock, flags);
 
-	if (gc->bgpio_dir_inverted)
-		gc->bgpio_dir |= bgpio_line2mask(gc, gpio);
-	else
-		gc->bgpio_dir &= ~bgpio_line2mask(gc, gpio);
-	gc->write_reg(gc->reg_dir, gc->bgpio_dir);
+	gc->bgpio_dir &= ~bgpio_line2mask(gc, gpio);
+
+	if (gc->reg_dir_in)
+		gc->write_reg(gc->reg_dir_in, ~gc->bgpio_dir);
+	if (gc->reg_dir_out)
+		gc->write_reg(gc->reg_dir_out, gc->bgpio_dir);
 
 	spin_unlock_irqrestore(&gc->bgpio_lock, flags);
 
@@ -385,11 +386,16 @@ static int bgpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
 
 static int bgpio_get_dir(struct gpio_chip *gc, unsigned int gpio)  {
-	/* Return 0 if output, 1 of input */
-	if (gc->bgpio_dir_inverted)
-		return !!(gc->read_reg(gc->reg_dir) & bgpio_line2mask(gc, gpio));
-	else
-		return !(gc->read_reg(gc->reg_dir) & bgpio_line2mask(gc, gpio));
+	/* Return 0 if output, 1 if input */
+	if (gc->bgpio_dir_unreadable)
+		return !(gc->bgpio_dir & bgpio_line2mask(gc, gpio));
+	if (gc->reg_dir_out)
+		return !(gc->read_reg(gc->reg_dir_out) & bgpio_line2mask(gc, gpio));
+	if (gc->reg_dir_in)
+		return !!(gc->read_reg(gc->reg_dir_in) & bgpio_line2mask(gc, gpio));
+
+	/* This should not happen */
+	return 1;
 }
 
 static int bgpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val) @@ -400,11 +406,12 @@ static int bgpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
 
 	spin_lock_irqsave(&gc->bgpio_lock, flags);
 
-	if (gc->bgpio_dir_inverted)
-		gc->bgpio_dir &= ~bgpio_line2mask(gc, gpio);
-	else
-		gc->bgpio_dir |= bgpio_line2mask(gc, gpio);
-	gc->write_reg(gc->reg_dir, gc->bgpio_dir);
+	gc->bgpio_dir |= bgpio_line2mask(gc, gpio);
+
+	if (gc->reg_dir_in)
+		gc->write_reg(gc->reg_dir_in, ~gc->bgpio_dir);
+	if (gc->reg_dir_out)
+		gc->write_reg(gc->reg_dir_out, gc->bgpio_dir);
 
 	spin_unlock_irqrestore(&gc->bgpio_lock, flags);
 
@@ -537,19 +544,19 @@ static int bgpio_setup_direction(struct gpio_chip *gc,
 				 void __iomem *dirin,
 				 unsigned long flags)
 {
-	if (dirout && dirin) {
-		return -EINVAL;
-	} else if (dirout) {
-		gc->reg_dir = dirout;
-		gc->direction_output = bgpio_dir_out;
-		gc->direction_input = bgpio_dir_in;
-		gc->get_direction = bgpio_get_dir;
-	} else if (dirin) {
-		gc->reg_dir = dirin;
+	if (dirout || dirin) {
+		gc->reg_dir_out = dirout;
+		gc->reg_dir_in = dirin;
 		gc->direction_output = bgpio_dir_out;
 		gc->direction_input = bgpio_dir_in;
 		gc->get_direction = bgpio_get_dir;
-		gc->bgpio_dir_inverted = true;
+		/*
+		 * If only dirin is available, this means we need
+		 * inverted semantics when handling get/set registers
+		 * so detect this here.
+		 */
+		if (dirin && !dirout)
+			gc->bgpio_dir_inverted = true;
 	} else {
 		if (flags & BGPIOF_NO_OUTPUT)
 			gc->direction_output = bgpio_dir_out_err; @@ -588,11 +595,11 @@ static int bgpio_request(struct gpio_chip *chip, unsigned gpio_pin)
  * @dirout: MMIO address for the register to set the line as OUTPUT. It is assumed
  *	that setting a line to 1 in this register will turn that line into an
  *	output line. Conversely, setting the line to 0 will turn that line into
- *	an input. Either this or @dirin can be defined, but never both.
+ *	an input.
  * @dirin: MMIO address for the register to set this line as INPUT. It is assumed
  *	that setting a line to 1 in this register will turn that line into an
  *	input line. Conversely, setting the line to 0 will turn that line into
- *	an output. Either this or @dirout can be defined, but never both.
+ *	an output.
  * @flags: Different flags that will affect the behaviour of the device, such as
  *	endianness etc.
  */
@@ -634,8 +641,28 @@ int bgpio_init(struct gpio_chip *gc, struct device *dev,
 	if (gc->set == bgpio_set_set &&
 			!(flags & BGPIOF_UNREADABLE_REG_SET))
 		gc->bgpio_data = gc->read_reg(gc->reg_set);
-	if (gc->reg_dir && !(flags & BGPIOF_UNREADABLE_REG_DIR))
-		gc->bgpio_dir = gc->read_reg(gc->reg_dir);
+
+	if (flags & BGPIOF_UNREADABLE_REG_DIR)
+		gc->bgpio_dir_unreadable = true;
+
+	/*
+	 * Inspect hardware to find initial direction setting.
+	 */
+	if ((gc->reg_dir_out || gc->reg_dir_in) &&
+	    !(flags & BGPIOF_UNREADABLE_REG_DIR)) {
+		if (gc->reg_dir_out)
+			gc->bgpio_dir = gc->read_reg(gc->reg_dir_out);
+		else if (gc->reg_dir_in)
+			gc->bgpio_dir = ~gc->read_reg(gc->reg_dir_in);
+		/*
+		 * If we have two direction registers, synchronise
+		 * input setting to output setting, the library
+		 * can not handle a line being input and output at
+		 * the same time.
+		 */
+		if (gc->reg_dir_out && gc->reg_dir_in)
+			gc->write_reg(gc->reg_dir_in, ~gc->bgpio_dir);
+	}
 
 	return ret;
 }
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h index 07cddbf45186..81dbcfd71245 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -227,9 +227,13 @@ struct gpio_irq_chip {
  * @reg_dat: data (in) register for generic GPIO
  * @reg_set: output set register (out=high) for generic GPIO
  * @reg_clr: output clear register (out=low) for generic GPIO
- * @reg_dir: direction setting register for generic GPIO
+ * @reg_dir_out: direction out setting register for generic GPIO
+ * @reg_dir_in: direction in setting register for generic GPIO
  * @bgpio_dir_inverted: indicates that the direction register is inverted
- *	(gpiolib private state variable)
+ *	(gpiolib private state variable) this means @reg_dir_in is
+ *	available but not @reg_dir_out.
+ * @bgpio_dir_unreadable: indicates that the direction register(s) cannot
+ *	be read and we need to rely on out internal state tracking.
  * @bgpio_bits: number of register bits used for a generic GPIO i.e.
  *	<register width> * 8
  * @bgpio_lock: used to lock chip->bgpio_data. Also, this is needed to keep @@ -237,7 +241,8 @@ struct gpio_irq_chip {
  * @bgpio_data:	shadowed data register for generic GPIO to clear/set bits
  *	safely.
  * @bgpio_dir: shadowed direction register for generic GPIO to clear/set
- *	direction safely.
+ *	direction safely. A "1" in this word means the line is set as
+ *	output.
  *
  * A gpio_chip can help platforms abstract various sources of GPIOs so
  * they can all be accessed through a common programing interface.
@@ -298,8 +303,10 @@ struct gpio_chip {
 	void __iomem *reg_dat;
 	void __iomem *reg_set;
 	void __iomem *reg_clr;
-	void __iomem *reg_dir;
+	void __iomem *reg_dir_out;
+	void __iomem *reg_dir_in;
 	bool bgpio_dir_inverted;
+	bool bgpio_dir_unreadable;
 	int bgpio_bits;
 	spinlock_t bgpio_lock;
 	unsigned long bgpio_data;
--
2.20.1