gpio: exar: refactor the driver

From: Bartosz Golaszewski <[email protected]>

We can simplify the code in gpio-exar by using regmap. This allows us to drop the mutex (regmap provides its own locking) and we can also reuse regmap's bit operations instead of implementing our own update function.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/gpio/Kconfig     |  1 +
 drivers/gpio/gpio-exar.c | 90 +++++++++++++++++-----------------------
 2 files changed, 38 insertions(+), 53 deletions(-)

drivers/gpio/Kconfig +1 -0

index 5d4de5cd6759..253a61ec9645 100644
@@ -255,6 +255,7 @@ config GPIO_EP93XX
 config GPIO_EXAR
 	tristate "Support for GPIO pins on XR17V352/354/358"
 	depends on SERIAL_8250_EXAR
+	select REGMAP_MMIO
 	help
 	  Selecting this option will enable handling of GPIO pins present
 	  on Exar XR17V352/354/358 chips.

drivers/gpio/gpio-exar.c +37 -52

index 28b0b4b5fa35..a2d324c513f8 100644
@@ -14,6 +14,7 @@
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
+#include <linux/regmap.h>
 
 #define EXAR_OFFSET_MPIOLVL_LO 0x90
 #define EXAR_OFFSET_MPIOSEL_LO 0x93

@@ -26,9 +27,8 @@ static DEFINE_IDA(ida_index);
 
 struct exar_gpio_chip {
 	struct gpio_chip gpio_chip;
-	struct mutex lock;
+	struct regmap *regs;
 	int index;
-	void __iomem *regs;
 	char name[20];
 	unsigned int first_pin;
 };

@@ -53,51 +53,13 @@ exar_offset_to_bit(struct exar_gpio_chip *exar_gpio, unsigned int offset)
 	return (offset + exar_gpio->first_pin) % 8;
 }
 
-static void exar_update(struct gpio_chip *chip, unsigned int reg, int val,
-			unsigned int offset)
-{
-	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
-	int temp;
-
-	mutex_lock(&exar_gpio->lock);
-	temp = readb(exar_gpio->regs + reg);
-	temp &= ~BIT(offset);
-	if (val)
-		temp |= BIT(offset);
-	writeb(temp, exar_gpio->regs + reg);
-	mutex_unlock(&exar_gpio->lock);
-}
-
-static int exar_set_direction(struct gpio_chip *chip, int direction,
-			      unsigned int offset)
-{
-	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
-	unsigned int addr = exar_offset_to_sel_addr(exar_gpio, offset);
-	unsigned int bit = exar_offset_to_bit(exar_gpio, offset);
-
-	exar_update(chip, addr, direction, bit);
-	return 0;
-}
-
-static int exar_get(struct gpio_chip *chip, unsigned int reg)
-{
-	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
-	int value;
-
-	mutex_lock(&exar_gpio->lock);
-	value = readb(exar_gpio->regs + reg);
-	mutex_unlock(&exar_gpio->lock);
-
-	return value;
-}
-
 static int exar_get_direction(struct gpio_chip *chip, unsigned int offset)
 {
 	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
 	unsigned int addr = exar_offset_to_sel_addr(exar_gpio, offset);
 	unsigned int bit = exar_offset_to_bit(exar_gpio, offset);
 
-	if (exar_get(chip, addr) & BIT(bit))
+	if (regmap_test_bits(exar_gpio->regs, addr, BIT(bit)))
 		return GPIO_LINE_DIRECTION_IN;
 
 	return GPIO_LINE_DIRECTION_OUT;

@@ -109,7 +71,7 @@ static int exar_get_value(struct gpio_chip *chip, unsigned int offset)
 	unsigned int addr = exar_offset_to_lvl_addr(exar_gpio, offset);
 	unsigned int bit = exar_offset_to_bit(exar_gpio, offset);
 
-	return !!(exar_get(chip, addr) & BIT(bit));
+	return !!(regmap_test_bits(exar_gpio->regs, addr, BIT(bit)));
 }
 
 static void exar_set_value(struct gpio_chip *chip, unsigned int offset,

@@ -119,21 +81,42 @@ static void exar_set_value(struct gpio_chip *chip, unsigned int offset,
 	unsigned int addr = exar_offset_to_lvl_addr(exar_gpio, offset);
 	unsigned int bit = exar_offset_to_bit(exar_gpio, offset);
 
-	exar_update(chip, addr, value, bit);
+	if (value)
+		regmap_set_bits(exar_gpio->regs, addr, BIT(bit));
+	else
+		regmap_clear_bits(exar_gpio->regs, addr, BIT(bit));
 }
 
 static int exar_direction_output(struct gpio_chip *chip, unsigned int offset,
 				 int value)
 {
+	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
+	unsigned int addr = exar_offset_to_sel_addr(exar_gpio, offset);
+	unsigned int bit = exar_offset_to_bit(exar_gpio, offset);
+
 	exar_set_value(chip, offset, value);
-	return exar_set_direction(chip, 0, offset);
+	regmap_clear_bits(exar_gpio->regs, addr, BIT(bit));
+
+	return 0;
 }
 
 static int exar_direction_input(struct gpio_chip *chip, unsigned int offset)
 {
-	return exar_set_direction(chip, 1, offset);
+	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
+	unsigned int addr = exar_offset_to_sel_addr(exar_gpio, offset);
+	unsigned int bit = exar_offset_to_bit(exar_gpio, offset);
+
+	regmap_set_bits(exar_gpio->regs, addr, BIT(bit));
+
+	return 0;
 }
 
+static const struct regmap_config exar_regmap_config = {
+	.name		= "exar-gpio",
+	.reg_bits	= 8,
+	.val_bits	= 8,
+};
+
 static int gpio_exar_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;

@@ -163,13 +146,17 @@ static int gpio_exar_probe(struct platform_device *pdev)
 	if (!exar_gpio)
 		return -ENOMEM;
 
-	mutex_init(&exar_gpio->lock);
+	/*
+	 * We don't need to check the return values of mmio regmap operations (unless
+	 * the regmap has a clock attached which is not the case here).
+	 */
+	exar_gpio->regs = devm_regmap_init_mmio(dev, p, &exar_regmap_config);
+	if (IS_ERR(exar_gpio->regs))
+		return PTR_ERR(exar_gpio->regs);
 
 	index = ida_alloc(&ida_index, GFP_KERNEL);
-	if (index < 0) {
-		ret = index;
-		goto err_mutex_destroy;
-	}
+	if (index < 0)
+		return index;
 
 	sprintf(exar_gpio->name, "exar_gpio%d", index);
 	exar_gpio->gpio_chip.label = exar_gpio->name;

@@ -195,8 +182,6 @@ static int gpio_exar_probe(struct platform_device *pdev)
 
 err_destroy:
 	ida_free(&ida_index, index);
-err_mutex_destroy:
-	mutex_destroy(&exar_gpio->lock);
 	return ret;
 }
 

Unfortunately, this one still crashes:

Quoted 4 lines · Jan Kiszka

Unfortunately, this one still crashes:

Just to confirm: does patch 5/7 alone work?

        int index;
-       void __iomem *regs;
        char name[20];
        unsigned int first_pin;

};

On Tue, Nov 10, 2020 at 2:35 PM Bartosz Golaszewski <[email protected]> wrote:

...

drivers/gpio/gpio-exar.c static DEFINE_IDA(ida_index); · line 30

Quoted 5 lines

 struct exar_gpio_chip {
        struct gpio_chip gpio_chip;

drivers/gpio/gpio-exar.c +1 -1

-       struct mutex lock;
 
+       struct regmap *regs;

Leaving the same name is a call for potential troubles.

Quoted 5 lines

        int index;
-       void __iomem *regs;
        char name[20];
        unsigned int first_pin;

};

...

drivers/gpio/gpio-exar.c static void exar_set_value(struct gpio_chip *chip, unsigned int offset, · line 118

Quoted 5 lines

drivers/gpio/gpio-exar.c +5 -0

+static const struct regmap_config exar_regmap_config = {
+       .name           = "exar-gpio",
+       .reg_bits       = 8,
+       .val_bits       = 8,
+};

Looking at the crash, are you sure this is a comprehensive description? Maybe it requires something like stride or so?

From: Bartosz Golaszewski <[email protected]>

We can simplify the code in gpio-exar by using regmap. This allows us to drop the mutex (regmap provides its own locking) and we can also reuse regmap's bit operations instead of implementing our own update function.

drivers/gpio/gpio-exar.c +5 -0

+	/*
+	 * We don't need to check the return values of mmio regmap operations (unless
+	 * the regmap has a clock attached which is not the case here).
+	 */
+	exar_gpio->regs = devm_regmap_init_mmio(dev, p, &exar_regmap_config);

Quoted 5 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

We can simplify the code in gpio-exar by using regmap. This allows us to drop the mutex (regmap provides its own locking) and we can also reuse regmap's bit operations instead of implementing our own update function.

...

Quoted 5 lines

drivers/gpio/gpio-exar.c +5 -0

+	/*
+	 * We don't need to check the return values of mmio regmap operations (unless
+	 * the regmap has a clock attached which is not the case here).
+	 */
+	exar_gpio->regs = devm_regmap_init_mmio(dev, p, &exar_regmap_config);

drivers/gpio/gpio-exar.c static int gpio_exar_probe(struct platform_device *pdev) · line 159

Quoted 10 lines

drivers/gpio/gpio-exar.c +4 -4

+	if (IS_ERR(exar_gpio->regs))
+		return PTR_ERR(exar_gpio->regs);
 
 	index = ida_alloc(&ida_index, GFP_KERNEL);
-	if (index < 0) {
-		ret = index;
-		goto err_mutex_destroy;
-	}
+	if (index < 0)
+		return index;

And below you effectively use p as regmap!
That's what renaming of variable regs -> regmap or map can easily reveal.

	exar_gpio->regs = p;

Quoted 25 lines · Andy Shevchenko

Quoted 5 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

We can simplify the code in gpio-exar by using regmap. This allows us to drop the mutex (regmap provides its own locking) and we can also reuse regmap's bit operations instead of implementing our own update function.

...

Quoted 15 lines

drivers/gpio/gpio-exar.c +9 -4

+   /*
+    * We don't need to check the return values of mmio regmap operations (unless
+    * the regmap has a clock attached which is not the case here).
+    */
+   exar_gpio->regs = devm_regmap_init_mmio(dev, p, &exar_regmap_config);
+   if (IS_ERR(exar_gpio->regs))
+           return PTR_ERR(exar_gpio->regs);
 
    index = ida_alloc(&ida_index, GFP_KERNEL);
-   if (index < 0) {
-           ret = index;
-           goto err_mutex_destroy;
-   }
+   if (index < 0)
+           return index;

Quoted 4 lines

And below you effectively use p as regmap!
That's what renaming of variable regs -> regmap or map can easily reveal.

      exar_gpio->regs = p;

Jan, if you remove this line, does it help?

Quoted 32 lines · Andy Shevchenko

Quoted 29 lines · Andy Shevchenko

Quoted 5 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

We can simplify the code in gpio-exar by using regmap. This allows us to drop the mutex (regmap provides its own locking) and we can also reuse regmap's bit operations instead of implementing our own update function.

...

Quoted 5 lines

drivers/gpio/gpio-exar.c +5 -0

+   /*
+    * We don't need to check the return values of mmio regmap operations (unless
+    * the regmap has a clock attached which is not the case here).
+    */
+   exar_gpio->regs = devm_regmap_init_mmio(dev, p, &exar_regmap_config);

drivers/gpio/gpio-exar.c static int gpio_exar_probe(struct platform_device *pdev) · line 159

Quoted 10 lines

drivers/gpio/gpio-exar.c +4 -4

+   if (IS_ERR(exar_gpio->regs))
+           return PTR_ERR(exar_gpio->regs);
 
    index = ida_alloc(&ida_index, GFP_KERNEL);
-   if (index < 0) {
-           ret = index;
-           goto err_mutex_destroy;
-   }
+   if (index < 0)
+           return index;

And below you effectively use p as regmap!
That's what renaming of variable regs -> regmap or map can easily reveal.

      exar_gpio->regs = p;

Quoted 2 lines

Jan, if you remove this line, does it help?

Ha! I guess you were right saying that keeping the name is asking for trouble then. :)

I think that may be it but address width should still be changed to 16.

Quoted 40 lines · Bartosz Golaszewski

Quoted 27 lines · Andy Shevchenko

Quoted 25 lines · Andy Shevchenko

Quoted 5 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

We can simplify the code in gpio-exar by using regmap. This allows us to drop the mutex (regmap provides its own locking) and we can also reuse regmap's bit operations instead of implementing our own update function.

...

Quoted 15 lines

drivers/gpio/gpio-exar.c +9 -4

+   /*
+    * We don't need to check the return values of mmio regmap operations (unless
+    * the regmap has a clock attached which is not the case here).
+    */
+   exar_gpio->regs = devm_regmap_init_mmio(dev, p, &exar_regmap_config);
+   if (IS_ERR(exar_gpio->regs))
+           return PTR_ERR(exar_gpio->regs);
 
    index = ida_alloc(&ida_index, GFP_KERNEL);
-   if (index < 0) {
-           ret = index;
-           goto err_mutex_destroy;
-   }
+   if (index < 0)
+           return index;

Quoted 7 lines

Quoted 4 lines

And below you effectively use p as regmap!
That's what renaming of variable regs -> regmap or map can easily reveal.

      exar_gpio->regs = p;

Jan, if you remove this line, does it help?

Ha! I guess you were right saying that keeping the name is asking for trouble then. :)

Quoted 2 lines

I think that may be it but address width should still be changed to 16.

Removing the line that Andy found made things work here. And switching to 16 for reg_bits didn't make things worse again.

Jan

Quoted 1 line · Jan Kiszka

On 10.11.20 15:30, Bartosz Golaszewski wrote:

Quoted 4 lines

Removing the line that Andy found made things work here. And switching to 16 for reg_bits didn't make things worse again.

Jan

Alright! I'll send a v4 with these things fixed then.