[PATCH 0/7] gpio: exar: refactor the driver

[PATCH 0/7] gpio: exar: refactor the driver

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

I just wanted to convert the driver to using simpler IDA API but ended up
quickly converting it to using regmap. Unfortunately I don't have the HW
to test it so marking the patches that introduce functional change as RFT
and Cc'ing the original author.

Bartosz Golaszewski (7):
  gpio: exar: add a newline after the copyright notice
  gpio: exar: include idr.h
  gpio: exar: switch to a simpler IDA interface
  gpio: exar: use a helper variable for &pdev->dev
  gpio: exar: unduplicate address and offset computation
  gpio: exar: switch to using regmap
  gpio: exar: use devm action for freeing the IDA and drop remove()

 drivers/gpio/Kconfig     |   1 +
 drivers/gpio/gpio-exar.c | 155 +++++++++++++++++++--------------------
 2 files changed, 77 insertions(+), 79 deletions(-)

-- 
2.29.1

[PATCH 1/7] gpio: exar: add a newline after the copyright notice

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

It's customary to have a newline between the copyright header and the
includes. Add one to gpio-exar.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-exar.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index b1accfba017d..4202dd363a11 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -4,6 +4,7 @@
  *
  * Copyright (C) 2015 Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
  */
+
 #include <linux/bitops.h>
 #include <linux/device.h>
 #include <linux/gpio/driver.h>
-- 
2.29.1

[PATCH 2/7] gpio: exar: include idr.h

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

This driver uses IDA APIs but doesn't include the relevant header. This
fixes it.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-exar.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index 4202dd363a11..1941ae533418 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -8,6 +8,7 @@
 #include <linux/bitops.h>
 #include <linux/device.h>
 #include <linux/gpio/driver.h>
+#include <linux/idr.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
-- 
2.29.1

[PATCH 3/7] gpio: exar: switch to a simpler IDA interface

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

We don't need to specify any ranges when allocating IDs so we can switch
to ida_alloc() and ida_free() instead of the ida_simple_ counterparts.

ida_simple_get(ida, 0, 0, gfp) is equivalent to
ida_alloc_range(ida, 0, UINT_MAX, gfp) which is equivalent to
ida_alloc(ida, gfp). Note: IDR will never actually allocate an ID
larger than INT_MAX.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-exar.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index 1941ae533418..752e8437ff80 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -149,7 +149,7 @@ static int gpio_exar_probe(struct platform_device *pdev)
 
 	mutex_init(&exar_gpio->lock);
 
-	index = ida_simple_get(&ida_index, 0, 0, GFP_KERNEL);
+	index = ida_alloc(&ida_index, GFP_KERNEL);
 	if (index < 0) {
 		ret = index;
 		goto err_mutex_destroy;
@@ -179,7 +179,7 @@ static int gpio_exar_probe(struct platform_device *pdev)
 	return 0;
 
 err_destroy:
-	ida_simple_remove(&ida_index, index);
+	ida_free(&ida_index, index);
 err_mutex_destroy:
 	mutex_destroy(&exar_gpio->lock);
 	return ret;
@@ -189,7 +189,7 @@ static int gpio_exar_remove(struct platform_device *pdev)
 {
 	struct exar_gpio_chip *exar_gpio = platform_get_drvdata(pdev);
 
-	ida_simple_remove(&ida_index, exar_gpio->index);
+	ida_free(&ida_index, exar_gpio->index);
 	mutex_destroy(&exar_gpio->lock);
 
 	return 0;
-- 
2.29.1

[PATCH 4/7] gpio: exar: use a helper variable for &pdev->dev

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

It's more elegant to use a helper local variable to store the address
of the underlying struct device than to dereference pdev everywhere. It
also has the benefit of avoiding unnecessary line breaks.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-exar.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index 752e8437ff80..db366d85b6b4 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -120,7 +120,8 @@ static int exar_direction_input(struct gpio_chip *chip, unsigned int offset)
 
 static int gpio_exar_probe(struct platform_device *pdev)
 {
-	struct pci_dev *pcidev = to_pci_dev(pdev->dev.parent);
+	struct device *dev = &pdev->dev;
+	struct pci_dev *pcidev = to_pci_dev(dev->parent);
 	struct exar_gpio_chip *exar_gpio;
 	u32 first_pin, ngpios;
 	void __iomem *p;
@@ -134,16 +135,15 @@ static int gpio_exar_probe(struct platform_device *pdev)
 	if (!p)
 		return -ENOMEM;
 
-	ret = device_property_read_u32(&pdev->dev, "exar,first-pin",
-				       &first_pin);
+	ret = device_property_read_u32(dev, "exar,first-pin", &first_pin);
 	if (ret)
 		return ret;
 
-	ret = device_property_read_u32(&pdev->dev, "ngpios", &ngpios);
+	ret = device_property_read_u32(dev, "ngpios", &ngpios);
 	if (ret)
 		return ret;
 
-	exar_gpio = devm_kzalloc(&pdev->dev, sizeof(*exar_gpio), GFP_KERNEL);
+	exar_gpio = devm_kzalloc(dev, sizeof(*exar_gpio), GFP_KERNEL);
 	if (!exar_gpio)
 		return -ENOMEM;
 
@@ -157,7 +157,7 @@ static int gpio_exar_probe(struct platform_device *pdev)
 
 	sprintf(exar_gpio->name, "exar_gpio%d", index);
 	exar_gpio->gpio_chip.label = exar_gpio->name;
-	exar_gpio->gpio_chip.parent = &pdev->dev;
+	exar_gpio->gpio_chip.parent = dev;
 	exar_gpio->gpio_chip.direction_output = exar_direction_output;
 	exar_gpio->gpio_chip.direction_input = exar_direction_input;
 	exar_gpio->gpio_chip.get_direction = exar_get_direction;
@@ -169,8 +169,7 @@ static int gpio_exar_probe(struct platform_device *pdev)
 	exar_gpio->index = index;
 	exar_gpio->first_pin = first_pin;
 
-	ret = devm_gpiochip_add_data(&pdev->dev,
-				     &exar_gpio->gpio_chip, exar_gpio);
+	ret = devm_gpiochip_add_data(dev, &exar_gpio->gpio_chip, exar_gpio);
 	if (ret)
 		goto err_destroy;
 
-- 
2.29.1

[RFT PATCH 5/7] gpio: exar: unduplicate address and offset computation

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Provide and use helpers for calculating the register address and bit
offset instead of hand coding it in every function.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-exar.c | 40 ++++++++++++++++++++++++++++------------
 1 file changed, 28 insertions(+), 12 deletions(-)

diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index db366d85b6b4..629f4dad6919 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -33,6 +33,26 @@ struct exar_gpio_chip {
 	unsigned int first_pin;
 };
 
+static unsigned int
+exar_offset_to_sel_addr(struct exar_gpio_chip *exar_gpio, unsigned int offset)
+{
+	return (offset + exar_gpio->first_pin) / 8 ? EXAR_OFFSET_MPIOSEL_HI
+						   : EXAR_OFFSET_MPIOSEL_LO;
+}
+
+static unsigned int
+exar_offset_to_lvl_addr(struct exar_gpio_chip *exar_gpio, unsigned int offset)
+{
+	return (offset + exar_gpio->first_pin) / 8 ? EXAR_OFFSET_MPIOLVL_HI
+						   : EXAR_OFFSET_MPIOLVL_LO;
+}
+
+static unsigned int
+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)
 {
@@ -52,9 +72,8 @@ 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 = (offset + exar_gpio->first_pin) / 8 ?
-		EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO;
-	unsigned int bit  = (offset + exar_gpio->first_pin) % 8;
+	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;
@@ -75,9 +94,8 @@ static int exar_get(struct gpio_chip *chip, unsigned int reg)
 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 = (offset + exar_gpio->first_pin) / 8 ?
-		EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO;
-	unsigned int bit  = (offset + exar_gpio->first_pin) % 8;
+	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))
 		return GPIO_LINE_DIRECTION_IN;
@@ -88,9 +106,8 @@ static int exar_get_direction(struct gpio_chip *chip, unsigned int offset)
 static int exar_get_value(struct gpio_chip *chip, unsigned int offset)
 {
 	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
-	unsigned int addr = (offset + exar_gpio->first_pin) / 8 ?
-		EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO;
-	unsigned int bit  = (offset + exar_gpio->first_pin) % 8;
+	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));
 }
@@ -99,9 +116,8 @@ static void exar_set_value(struct gpio_chip *chip, unsigned int offset,
 			   int value)
 {
 	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
-	unsigned int addr = (offset + exar_gpio->first_pin) / 8 ?
-		EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO;
-	unsigned int bit  = (offset + exar_gpio->first_pin) % 8;
+	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, value, bit);
 }
-- 
2.29.1

[RFT PATCH 6/7] gpio: exar: switch to using regmap

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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 <bgolaszewski@baylibre.com>
---
 drivers/gpio/Kconfig     |  1 +
 drivers/gpio/gpio-exar.c | 91 ++++++++++++++++------------------------
 2 files changed, 38 insertions(+), 54 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 5d4de5cd6759..253a61ec9645 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -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.
diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index 629f4dad6919..e4da278f74a2 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -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,41 @@ static void exar_set_value(struct gpio_chip *chip, unsigned int offset,
 	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, 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)
 {
-	exar_set_value(chip, offset, value);
-	return exar_set_direction(chip, 0, 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_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 +145,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 +181,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;
 }
 
@@ -205,7 +189,6 @@ static int gpio_exar_remove(struct platform_device *pdev)
 	struct exar_gpio_chip *exar_gpio = platform_get_drvdata(pdev);
 
 	ida_free(&ida_index, exar_gpio->index);
-	mutex_destroy(&exar_gpio->lock);
 
 	return 0;
 }
-- 
2.29.1

[PATCH 7/7] gpio: exar: use devm action for freeing the IDA and drop remove()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

We can simplify the error path in probe() and drop remove() entirely if
we provide a devm action for freeing the device ID.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-exar.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index e4da278f74a2..fda9346d1dc0 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -110,6 +110,13 @@ static int exar_direction_input(struct gpio_chip *chip, unsigned int offset)
 	return 0;
 }
 
+static void exar_devm_ida_free(void *data)
+{
+	struct exar_gpio_chip *exar_gpio = data;
+
+	ida_free(&ida_index, exar_gpio->index);
+}
+
 static const struct regmap_config exar_regmap_config = {
 	.name		= "exar-gpio",
 	.reg_bits	= 8,
@@ -157,6 +164,10 @@ static int gpio_exar_probe(struct platform_device *pdev)
 	if (index < 0)
 		return index;
 
+	ret = devm_add_action_or_reset(dev, exar_devm_ida_free, exar_gpio);
+	if (ret)
+		return ret;
+
 	sprintf(exar_gpio->name, "exar_gpio%d", index);
 	exar_gpio->gpio_chip.label = exar_gpio->name;
 	exar_gpio->gpio_chip.parent = dev;
@@ -173,29 +184,15 @@ static int gpio_exar_probe(struct platform_device *pdev)
 
 	ret = devm_gpiochip_add_data(dev, &exar_gpio->gpio_chip, exar_gpio);
 	if (ret)
-		goto err_destroy;
+		return ret;
 
 	platform_set_drvdata(pdev, exar_gpio);
 
-	return 0;
-
-err_destroy:
-	ida_free(&ida_index, index);
-	return ret;
-}
-
-static int gpio_exar_remove(struct platform_device *pdev)
-{
-	struct exar_gpio_chip *exar_gpio = platform_get_drvdata(pdev);
-
-	ida_free(&ida_index, exar_gpio->index);
-
 	return 0;
 }
 
 static struct platform_driver gpio_exar_driver = {
 	.probe	= gpio_exar_probe,
-	.remove	= gpio_exar_remove,
 	.driver	= {
 		.name = DRIVER_NAME,
 	},
-- 
2.29.1

Re: [PATCH 0/7] gpio: exar: refactor the driver

[Andy Shevchenko]

On Mon, Oct 26, 2020 at 4:22 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> I just wanted to convert the driver to using simpler IDA API but ended up
> quickly converting it to using regmap. Unfortunately I don't have the HW
> to test it so marking the patches that introduce functional change as RFT
> and Cc'ing the original author.

+Cc: Jan, AFAIR their devices are using Exar UART.

> Bartosz Golaszewski (7):
>   gpio: exar: add a newline after the copyright notice
>   gpio: exar: include idr.h
>   gpio: exar: switch to a simpler IDA interface
>   gpio: exar: use a helper variable for &pdev->dev
>   gpio: exar: unduplicate address and offset computation
>   gpio: exar: switch to using regmap
>   gpio: exar: use devm action for freeing the IDA and drop remove()
>
>  drivers/gpio/Kconfig     |   1 +
>  drivers/gpio/gpio-exar.c | 155 +++++++++++++++++++--------------------
>  2 files changed, 77 insertions(+), 79 deletions(-)
>
> --
> 2.29.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [RFT PATCH 5/7] gpio: exar: unduplicate address and offset computation

[Andy Shevchenko]

On Mon, Oct 26, 2020 at 4:23 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Provide and use helpers for calculating the register address and bit
> offset instead of hand coding it in every function.

Can you check code generation on x86, for example?

Sometimes compilers are eager to use idiv assembly instruction which
does simultaneously / and %.
I dunno if a) it's used for / 8 and % 8 since 8 is 2^3, b) splitting
to functions makes the above optimisation impossible.

> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/gpio/gpio-exar.c | 40 ++++++++++++++++++++++++++++------------
>  1 file changed, 28 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
> index db366d85b6b4..629f4dad6919 100644
> --- a/drivers/gpio/gpio-exar.c
> +++ b/drivers/gpio/gpio-exar.c
> @@ -33,6 +33,26 @@ struct exar_gpio_chip {
>         unsigned int first_pin;
>  };
>
> +static unsigned int
> +exar_offset_to_sel_addr(struct exar_gpio_chip *exar_gpio, unsigned int offset)
> +{
> +       return (offset + exar_gpio->first_pin) / 8 ? EXAR_OFFSET_MPIOSEL_HI
> +                                                  : EXAR_OFFSET_MPIOSEL_LO;
> +}
> +
> +static unsigned int
> +exar_offset_to_lvl_addr(struct exar_gpio_chip *exar_gpio, unsigned int offset)
> +{
> +       return (offset + exar_gpio->first_pin) / 8 ? EXAR_OFFSET_MPIOLVL_HI
> +                                                  : EXAR_OFFSET_MPIOLVL_LO;
> +}
> +
> +static unsigned int
> +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)
>  {
> @@ -52,9 +72,8 @@ 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 = (offset + exar_gpio->first_pin) / 8 ?
> -               EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO;
> -       unsigned int bit  = (offset + exar_gpio->first_pin) % 8;
> +       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;
> @@ -75,9 +94,8 @@ static int exar_get(struct gpio_chip *chip, unsigned int reg)
>  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 = (offset + exar_gpio->first_pin) / 8 ?
> -               EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO;
> -       unsigned int bit  = (offset + exar_gpio->first_pin) % 8;
> +       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))
>                 return GPIO_LINE_DIRECTION_IN;
> @@ -88,9 +106,8 @@ static int exar_get_direction(struct gpio_chip *chip, unsigned int offset)
>  static int exar_get_value(struct gpio_chip *chip, unsigned int offset)
>  {
>         struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
> -       unsigned int addr = (offset + exar_gpio->first_pin) / 8 ?
> -               EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO;
> -       unsigned int bit  = (offset + exar_gpio->first_pin) % 8;
> +       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));
>  }
> @@ -99,9 +116,8 @@ static void exar_set_value(struct gpio_chip *chip, unsigned int offset,
>                            int value)
>  {
>         struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
> -       unsigned int addr = (offset + exar_gpio->first_pin) / 8 ?
> -               EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO;
> -       unsigned int bit  = (offset + exar_gpio->first_pin) % 8;
> +       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, value, bit);
>  }
> --
> 2.29.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [RFT PATCH 6/7] gpio: exar: switch to using regmap

[Andy Shevchenko]

On Mon, Oct 26, 2020 at 4:23 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> 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.

...

> +       if (value)
> +               regmap_set_bits(exar_gpio->regs, addr, BIT(bit));
> +       else
> +               regmap_clear_bits(exar_gpio->regs, addr, BIT(bit));

A side note: perhaps + regmap_assign_bits() ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 7/7] gpio: exar: use devm action for freeing the IDA and drop remove()

[Andy Shevchenko]

On Mon, Oct 26, 2020 at 4:22 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> We can simplify the error path in probe() and drop remove() entirely if
> we provide a devm action for freeing the device ID.

Always the same question to IDR/IDA users:
does it guarantee that when the driver is gone the IDR/IDA resources are freed?

(It's not directly related to this patch, though)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/7] gpio: exar: refactor the driver

[Andy Shevchenko]

On Mon, Oct 26, 2020 at 4:46 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, Oct 26, 2020 at 4:22 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > I just wanted to convert the driver to using simpler IDA API but ended up
> > quickly converting it to using regmap. Unfortunately I don't have the HW
> > to test it so marking the patches that introduce functional change as RFT
> > and Cc'ing the original author.
>
> +Cc: Jan, AFAIR their devices are using Exar UART.

From code perspective looks good to me, FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

(One nit to one patch, but I think it should be fine)

> > Bartosz Golaszewski (7):
> >   gpio: exar: add a newline after the copyright notice
> >   gpio: exar: include idr.h
> >   gpio: exar: switch to a simpler IDA interface
> >   gpio: exar: use a helper variable for &pdev->dev
> >   gpio: exar: unduplicate address and offset computation
> >   gpio: exar: switch to using regmap
> >   gpio: exar: use devm action for freeing the IDA and drop remove()


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 7/7] gpio: exar: use devm action for freeing the IDA and drop remove()

[Bartosz Golaszewski]

On Mon, Oct 26, 2020 at 4:02 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, Oct 26, 2020 at 4:22 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > We can simplify the error path in probe() and drop remove() entirely if
> > we provide a devm action for freeing the device ID.
>
> Always the same question to IDR/IDA users:
> does it guarantee that when the driver is gone the IDR/IDA resources are freed?
>
> (It's not directly related to this patch, though)
>

Yes because there's exactly one ida_free(id) per id = ida_alloc() here.

Bartosz

Re: [PATCH 0/7] gpio: exar: refactor the driver

[Jan Kiszka]

On 26.10.20 15:46, Andy Shevchenko wrote:
> On Mon, Oct 26, 2020 at 4:22 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>
>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>
>> I just wanted to convert the driver to using simpler IDA API but ended up
>> quickly converting it to using regmap. Unfortunately I don't have the HW
>> to test it so marking the patches that introduce functional change as RFT
>> and Cc'ing the original author.
> 
> +Cc: Jan, AFAIR their devices are using Exar UART.
> 

Thanks for CC'ing. I cannot promise testing this soon, but I will try my
best.

Jan

>> Bartosz Golaszewski (7):
>>   gpio: exar: add a newline after the copyright notice
>>   gpio: exar: include idr.h
>>   gpio: exar: switch to a simpler IDA interface
>>   gpio: exar: use a helper variable for &pdev->dev
>>   gpio: exar: unduplicate address and offset computation
>>   gpio: exar: switch to using regmap
>>   gpio: exar: use devm action for freeing the IDA and drop remove()
>>
>>  drivers/gpio/Kconfig     |   1 +
>>  drivers/gpio/gpio-exar.c | 155 +++++++++++++++++++--------------------
>>  2 files changed, 77 insertions(+), 79 deletions(-)
>>
>> --
>> 2.29.1
>>
> 

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [RFT PATCH 5/7] gpio: exar: unduplicate address and offset computation

[Bartosz Golaszewski]

On Mon, Oct 26, 2020 at 3:51 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, Oct 26, 2020 at 4:23 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > Provide and use helpers for calculating the register address and bit
> > offset instead of hand coding it in every function.
>
> Can you check code generation on x86, for example?
>
> Sometimes compilers are eager to use idiv assembly instruction which
> does simultaneously / and %.
> I dunno if a) it's used for / 8 and % 8 since 8 is 2^3, b) splitting
> to functions makes the above optimisation impossible.
>

Is this optimization really needed though? It's not like it's a hot
path if it's protected by a mutex anyway. I prefer cleaner code here.

Bartosz

Re: [RFT PATCH 5/7] gpio: exar: unduplicate address and offset computation

[Andy Shevchenko]

On Mon, Oct 26, 2020 at 4:23 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

...

> +static unsigned int
> +exar_offset_to_sel_addr(struct exar_gpio_chip *exar_gpio, unsigned int offset)
> +{
> +       return (offset + exar_gpio->first_pin) / 8 ? EXAR_OFFSET_MPIOSEL_HI
> +                                                  : EXAR_OFFSET_MPIOSEL_LO;
> +}
> +
> +static unsigned int
> +exar_offset_to_lvl_addr(struct exar_gpio_chip *exar_gpio, unsigned int offset)
> +{
> +       return (offset + exar_gpio->first_pin) / 8 ? EXAR_OFFSET_MPIOLVL_HI
> +                                                  : EXAR_OFFSET_MPIOLVL_LO;
> +}
> +
> +static unsigned int
> +exar_offset_to_bit(struct exar_gpio_chip *exar_gpio, unsigned int offset)
> +{
> +       return (offset + exar_gpio->first_pin) % 8;
> +}

Answering to your question...

It can be done line this:

static unsigned int exar_offset_to_bank_and_bit(..., *bit)
{
       *bit = (offset + exar_gpio->first_pin) % 8;
       return (offset + exar_gpio->first_pin) / 8;
}

static unsigned int exar_offset_to_lvl_addr_and_bit(, *bit)
{
    return exar_offset_to_bank_and_bit(..., bit) ?
        EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO;
}

...

> +       unsigned int addr = exar_offset_to_lvl_addr(exar_gpio, offset);
> +       unsigned int bit = exar_offset_to_bit(exar_gpio, offset);

unsigned int addr, bit;

addr = exar_offset_to_lvl_addr_and_bit(..., &bit);

-- 
With Best Regards,
Andy Shevchenko

RE: [RFT PATCH 5/7] gpio: exar: unduplicate address and offset computation

[David Laight]

From: Andy Shevchenko
> Sent: 02 November 2020 10:59
> 
> On Mon, Oct 26, 2020 at 4:23 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> 
> ...
> 
> > +static unsigned int
> > +exar_offset_to_sel_addr(struct exar_gpio_chip *exar_gpio, unsigned int offset)
> > +{
> > +       return (offset + exar_gpio->first_pin) / 8 ? EXAR_OFFSET_MPIOSEL_HI
> > +                                                  : EXAR_OFFSET_MPIOSEL_LO;
> > +}
> > +
> > +static unsigned int
> > +exar_offset_to_lvl_addr(struct exar_gpio_chip *exar_gpio, unsigned int offset)
> > +{
> > +       return (offset + exar_gpio->first_pin) / 8 ? EXAR_OFFSET_MPIOLVL_HI
> > +                                                  : EXAR_OFFSET_MPIOLVL_LO;
> > +}
> > +
> > +static unsigned int
> > +exar_offset_to_bit(struct exar_gpio_chip *exar_gpio, unsigned int offset)
> > +{
> > +       return (offset + exar_gpio->first_pin) % 8;
> > +}
> 
> Answering to your question...
> 
> It can be done line this:
> 
> static unsigned int exar_offset_to_bank_and_bit(..., *bit)
> {
>        *bit = (offset + exar_gpio->first_pin) % 8;
>        return (offset + exar_gpio->first_pin) / 8;
> }

That is likely to require the compiler reload exar_gpio->first_pin
after the write to *bit.

> static unsigned int exar_offset_to_lvl_addr_and_bit(, *bit)
> {
>     return exar_offset_to_bank_and_bit(..., bit) ?
>         EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO;
> }

Gah why is it using divide then ?: ?
AFAICT (from the above) there are at most 16 pins.

Much better would be using:
	tmp =	offset + exar_gpio->first_pin;
	*bit = tmp & 7;
	return tmp & 8;

Inlined the compiler may well compute:
	exar_offset_to_bank_and_bit() ? HI : LO;
as:
	LO + (HI - LO) * exar_offset_to_bank_and_bit().
The latter term is likely to be just (tmp & 8) >> n.

I also bet the code actually wants (1 << bit).

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 0/7] gpio: exar: refactor the driver

[Jan Kiszka]

On 27.10.20 16:12, Jan Kiszka wrote:
> On 26.10.20 15:46, Andy Shevchenko wrote:
>> On Mon, Oct 26, 2020 at 4:22 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>>
>>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>>
>>> I just wanted to convert the driver to using simpler IDA API but ended up
>>> quickly converting it to using regmap. Unfortunately I don't have the HW
>>> to test it so marking the patches that introduce functional change as RFT
>>> and Cc'ing the original author.
>>
>> +Cc: Jan, AFAIR their devices are using Exar UART.
>>
> 
> Thanks for CC'ing. I cannot promise testing this soon, but I will try my
> best.
> 

Finally tested, unfortunately with bad news:

...
at24 i2c-INT3499:00: 1024 byte INT3499:00 EEPROM, writable, 1 bytes/write
pxa2xx_spi_pci 0000:00:15.0: enabling device (0000 -> 0002)
pxa2xx_spi_pci 0000:00:15.1: enabling device (0000 -> 0002)
exar_serial 0000:02:00.0: enabling device (0000 -> 0002)
0000:02:00.0: ttyS2 at MMIO 0x90000000 (irq = 44, base_baud = 7812500) is a XR17V35X
0000:02:00.0: ttyS3 at MMIO 0x90000400 (irq = 44, base_baud = 7812500) is a XR17V35X
BUG: kernel NULL pointer dereference, address: 00000000
#PF: supervisor instruction fetch in kernel mode
#PF: error_code(0xc1150010) - not-present page
*pde = 00000000 
Oops: 0010 [#1] PREEMPT
CPU: 0 PID: 5 Comm: kworker/0:0 Not tainted 5.10.0-rc2+ #438
Hardware name: Intel Corp. QUARK/SIMATIC IOT2000, BIOS V24.02.01 10/30/2018
Workqueue: events deferred_probe_work_func
EIP: 0x0
Code: Unable to access opcode bytes at RIP 0xffffffd6.
EAX: 00000000 EBX: f7c74000 ECX: 00000004 EDX: 00000099
ESI: 00000000 EDI: 00000000 EBP: c1157da8 ESP: c1157d90
DS: 007b ES: 007b FS: 0000 GS: 00e0 SS: 0068 EFLAGS: 00010282
CR0: 80050033 CR2: ffffffd6 CR3: 03771000 CR4: 00100010
Call Trace:
 regmap_update_bits_base+0x22/0x60
 ? exar_set_value+0x70/0x70 [gpio_exar]
 ? exar_set_value+0x70/0x70 [gpio_exar]
 exar_direction_output+0x47/0x50 [gpio_exar]
 gpiod_direction_output_raw_commit+0x74/0x270
 ? exar_direction_input+0x50/0x50 [gpio_exar]
 ? exar_set_value+0x70/0x70 [gpio_exar]
 gpiod_direction_output+0xf0/0x160
 create_gpio_led+0xea/0x180
 gpio_led_probe+0x22c/0x460
 ? device_pm_check_callbacks+0x4c/0x100
 platform_drv_probe+0x2d/0x80
 really_probe+0xcb/0x330
 driver_probe_device+0x49/0xa0
 __device_attach_driver+0x61/0x80
 ? driver_allows_async_probing+0x60/0x60
 bus_for_each_drv+0x4f/0x90
 __device_attach+0xbb/0x120
 ? driver_allows_async_probing+0x60/0x60
 device_initial_probe+0x12/0x20
 bus_probe_device+0x6f/0x80
 deferred_probe_work_func+0x56/0x80
 process_one_work+0x1ce/0x390
 worker_thread+0x37/0x420
 kthread+0x115/0x130
 ? process_one_work+0x390/0x390
 ? kthread_create_on_node+0x20/0x20
 ret_from_fork+0x19/0x30
Modules linked in: gpio_exar(+) spi_pxa2xx_platform 8250_exar spi_pxa2xx_pci ti_adc108s102 industrialio_triggered_buffer kfifo_buf industrialio at24
CR2: 0000000000000000
---[ end trace d982fb210f759304 ]---
EIP: 0x0
Code: Unable to access opcode bytes at RIP 0xffffffd6.
EAX: 00000000 EBX: f7c74000 ECX: 00000004 EDX: 00000099
ESI: 00000000 EDI: 00000000 EBP: c1157da8 ESP: c1157d90
DS: 007b ES: 007b FS: 0000 GS: 00e0 SS: 0068 EFLAGS: 00010282
CR0: 80050033 CR2: ffffffd6 CR3: 03771000 CR4: 00100010

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH 0/7] gpio: exar: refactor the driver

[Andy Shevchenko]

On Wed, Nov 4, 2020 at 3:57 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 27.10.20 16:12, Jan Kiszka wrote:
> > On 26.10.20 15:46, Andy Shevchenko wrote:
> >> On Mon, Oct 26, 2020 at 4:22 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >>>
> >>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >>>
> >>> I just wanted to convert the driver to using simpler IDA API but ended up
> >>> quickly converting it to using regmap. Unfortunately I don't have the HW
> >>> to test it so marking the patches that introduce functional change as RFT
> >>> and Cc'ing the original author.
> >>
> >> +Cc: Jan, AFAIR their devices are using Exar UART.
> >>
> >
> > Thanks for CC'ing. I cannot promise testing this soon, but I will try my
> > best.
> >
>
> Finally tested, unfortunately with bad news:

> Code: Unable to access opcode bytes at RIP 0xffffffd6.

I guess it is due to missed error pointer handling somewhere, as above
is equal to -ENOMSG.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/7] gpio: exar: refactor the driver

[Bartosz Golaszewski]

On Wed, Nov 4, 2020 at 3:51 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Nov 4, 2020 at 3:57 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
> > On 27.10.20 16:12, Jan Kiszka wrote:
> > > On 26.10.20 15:46, Andy Shevchenko wrote:
> > >> On Mon, Oct 26, 2020 at 4:22 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >>>
> > >>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >>>
> > >>> I just wanted to convert the driver to using simpler IDA API but ended up
> > >>> quickly converting it to using regmap. Unfortunately I don't have the HW
> > >>> to test it so marking the patches that introduce functional change as RFT
> > >>> and Cc'ing the original author.
> > >>
> > >> +Cc: Jan, AFAIR their devices are using Exar UART.
> > >>
> > >
> > > Thanks for CC'ing. I cannot promise testing this soon, but I will try my
> > > best.
> > >
> >
> > Finally tested, unfortunately with bad news:
>
> > Code: Unable to access opcode bytes at RIP 0xffffffd6.
>
> I guess it is due to missed error pointer handling somewhere, as above
> is equal to -ENOMSG.
>

Yeah I'd guess it's the regmap pointer but we do check the return
value of regmap init with IS_ERR(). :/

Bartosz

Re: [RFT PATCH 6/7] gpio: exar: switch to using regmap

[Bartosz Golaszewski]

On Mon, Oct 26, 2020 at 3:18 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> 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 <bgolaszewski@baylibre.com>

[snip]

>
>  static int exar_direction_output(struct gpio_chip *chip, unsigned int offset,
>                                  int value)
>  {
> -       exar_set_value(chip, offset, value);
> -       return exar_set_direction(chip, 0, 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_clear_bits(exar_gpio->regs, addr, BIT(bit));
> +
> +       return 0;
>  }
>

Upon closer look I noticed this now ignores the value argument. I
doubt however it's the culprit of the crash Jan reported.

[snip]

Bartosz