gpio: exar: refactor the driver

PATCH Superseded Bartosz Golaszewski 3 participants 15 messages Nov 4, 2020 – Nov 6, 2020

Subsystems (2)

GPIO Register map abstraction

lore

Share & tools

lore

Bartosz Golaszewski

author

2020-11-04 19:30 UTC #0 lore raw

v2 cover gpio: exar: refactor the driver

From: Bartosz Golaszewski <[email protected]>

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.

v1 -> v2:
- add new regmap helper: regmap_assign_bits()
- fix lvl vs sel register access
- set value in direction_output callback

Note: I didn't use the fancy method of offset calculation Andy and David suggested because this series broke the driver according to Jan - let's get it right before we modify it any more. I found a couple problems that could maybe cause the crash. Jan: could you give it another spin?

Bartosz Golaszewski (8):
regmap: provide regmap_assign_bits()
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 | 151 +++++++++++++++++++--------------------
 include/linux/regmap.h   |  16 +++++
 3 files changed, 90 insertions(+), 78 deletions(-)

Signature · 1 line

2.29.1

Bartosz Golaszewski

author

2020-11-04 19:31 UTC #1 lore raw

PATCH v2 1/8 regmap: provide regmap_assign_bits()

From: Bartosz Golaszewski <[email protected]>

Add another bits helper to regmap API: this one sets given bits if value is true and clears them if it's false.

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

---
 include/linux/regmap.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

include/linux/regmap.h +16 -0

index e7834d98207f..62099e7a3ed6 100644
@@ -1148,6 +1148,15 @@ static inline int regmap_clear_bits(struct regmap *map,
 	return regmap_update_bits_base(map, reg, bits, 0, NULL, false, false);
 }
 
+static inline int regmap_assign_bits(struct regmap *map, unsigned int reg,
+				     unsigned int bits, bool value)
+{
+	if (value)
+		return regmap_set_bits(map, reg, bits);
+	else
+		return regmap_clear_bits(map, reg, bits);
+}
+
 int regmap_test_bits(struct regmap *map, unsigned int reg, unsigned int bits);
 
 /**

@@ -1554,6 +1563,13 @@ static inline int regmap_clear_bits(struct regmap *map,
 	return -EINVAL;
 }
 
+static inline int regmap_assign_bits(struct regmap *map, unsigned int reg,
+				     unsigned int bits, bool value)
+{
+	WARN_ONCE(1, "regmap API is disabled");
+	return -EINVAL;
+}
+
 static inline int regmap_test_bits(struct regmap *map,
 				   unsigned int reg, unsigned int bits)
 {

Signature · 1 line

2.29.1

Bartosz Golaszewski

author

2020-11-04 19:31 UTC #2 lore raw

PATCH v2 2/8 gpio: exar: add a newline after the copyright notice

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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

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

---
 drivers/gpio/gpio-exar.c | 1 +
 1 file changed, 1 insertion(+)

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

index b1accfba017d..4202dd363a11 100644
@@ -4,6 +4,7 @@
  *
  * Copyright (C) 2015 Sudip Mukherjee <[email protected]>
  */
+
 #include <linux/bitops.h>
 #include <linux/device.h>
 #include <linux/gpio/driver.h>

Signature · 1 line

2.29.1

Bartosz Golaszewski

author

2020-11-04 19:31 UTC #3 lore raw

PATCH v2 3/8 gpio: exar: include idr.h

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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

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

---
 drivers/gpio/gpio-exar.c | 1 +
 1 file changed, 1 insertion(+)

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

index 4202dd363a11..1941ae533418 100644
@@ -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>

Signature · 1 line

2.29.1

Bartosz Golaszewski

author

2020-11-04 19:31 UTC #4 lore raw

PATCH v2 4/8 gpio: exar: switch to a simpler IDA interface

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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 <[email protected]>

---
 drivers/gpio/gpio-exar.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

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

index 1941ae533418..752e8437ff80 100644
@@ -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;

Signature · 1 line

2.29.1

Bartosz Golaszewski

author

2020-11-04 19:31 UTC #5 lore raw

PATCH v2 5/8 gpio: exar: use a helper variable for &pdev->dev

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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 <[email protected]>

---
 drivers/gpio/gpio-exar.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

drivers/gpio/gpio-exar.c +7 -8

index 752e8437ff80..db366d85b6b4 100644
@@ -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;

Signature · 1 line

2.29.1

Bartosz Golaszewski

author

2020-11-04 19:31 UTC #6 lore raw

PATCH v2 6/8 gpio: exar: unduplicate address and offset computation

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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 <[email protected]>

---
 drivers/gpio/gpio-exar.c | 40 ++++++++++++++++++++++++++++------------
 1 file changed, 28 insertions(+), 12 deletions(-)

drivers/gpio/gpio-exar.c +28 ~~-12~~

index db366d85b6b4..28b0b4b5fa35 100644
@@ -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_lvl_addr(exar_gpio, offset);
+	unsigned int bit = exar_offset_to_bit(exar_gpio, offset);
 
 	exar_update(chip, addr, value, bit);
 }

Signature · 1 line

2.29.1

Bartosz Golaszewski

author

2020-11-04 19:31 UTC #7 lore raw

PATCH v2 7/8 gpio: exar: switch to using regmap

In reply to 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.

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

---
 drivers/gpio/Kconfig     |  1 +
 drivers/gpio/gpio-exar.c | 87 ++++++++++++++++------------------------
 2 files changed, 35 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 +34 ~~-53~~

156 lines not on this page · raw message

Signature · 1 line

2.29.1

Andy Shevchenko

2020-11-04 20:35 UTC #8 lore raw

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

On Wed, Nov 4, 2020 at 9:34 PM Bartosz Golaszewski <[email protected]> wrote:

...

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

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 gpio-pca953xx regmap conversion I'm wondering shouldn't you provide a callback to define volatile registers (such as GPIO input bits)?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Bartosz Golaszewski

author

2020-11-05 08:56 UTC #9 lore raw

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

Quoted 15 lines · Andy Shevchenko

On Wed, Nov 4, 2020 at 9:34 PM Bartosz Golaszewski <[email protected]> wrote:
...
drivers/gpio/gpio-exar.c static void exar_set_value(struct gpio_chip *chip, unsigned int offset, · line 115
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 gpio-pca953xx regmap conversion I'm wondering shouldn't you provide a callback to define volatile registers (such as GPIO input bits)?

Quoted 3 lines

--
With Best Regards,
Andy Shevchenko

I think this was done in pca953x due to weird calculations of banks and registers. For a rather simple driver like this one I don't think this is needed.

Bartosz

Mark Brown

maintainer

2020-11-05 17:41 UTC #10 lore raw

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

In reply to Bartosz Golaszewski

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

Quoted 7 lines · Bartosz Golaszewski

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

@@ -119,21 +81,39 @@ 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);
+	regmap_assign_bits(exar_gpio->regs, addr, BIT(bit), value);
 }

This appears to be the use of _assign_bits() and TBH I'm still both having a hard time understanding the motivation for it and liking the name, especially since AFAICT it's only setting a single bit here. The above is just

	regmap_update_bits(exar_gpio->regs, addr, 1 << bit, value << bit);

AFAICT (and indeed now I dig around assign_bit() only works on a single bit and does both shifts which makes the correspondance with that interface super unclear, we're not mirroring that interface here). If you're trying to clone the bitops function it should probably be an actual clone of the bitops function not something different, that would be clearer and it'd be easier to understand why someone would want the API in the first place. But perhaps I'm missing something here?

Bartosz Golaszewski

author

2020-11-06 11:14 UTC #11 lore raw

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

Quoted 18 lines · Mark Brown

drivers/gpio/gpio-exar.c static void exar_set_value(struct gpio_chip *chip, unsigned int offset, · line 85
Quoted 7 lines · Bartosz Golaszewski
drivers/gpio/gpio-exar.c +1 -1
@@ -119,21 +81,39 @@ 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);
+     regmap_assign_bits(exar_gpio->regs, addr, BIT(bit), value);
 }
This appears to be the use of _assign_bits() and TBH I'm still both having a hard time understanding the motivation for it and liking the name, especially since AFAICT it's only setting a single bit here. The above is just
        regmap_update_bits(exar_gpio->regs, addr, 1 << bit, value << bit);

Quoted 7 lines

AFAICT (and indeed now I dig around assign_bit() only works on a single bit and does both shifts which makes the correspondance with that interface super unclear, we're not mirroring that interface here). If you're trying to clone the bitops function it should probably be an actual clone of the bitops function not something different, that would be clearer and it'd be easier to understand why someone would want the API in the first place. But perhaps I'm missing something here?

It's true that bitops set/clear/assign bit macros work on single bits and take their offsets as arguments. However all regmap helpers operate on masks. Two release cycles back we added two helpers regmap_set_bits() and regmap_clear_bits() which are just wrappers around regmap_update_bits(). The naming was inspired by bitops (because how would one name these operations differently anyway?) but it was supposed to be able to clear/set multiple bits at once - at least this was my use-case in mtk-star-emac driver I was writing at the time and for which I wrote these helpers.

Now the regmap_assign_bits() helper is just an extension to these two which allows users to use one line instead of four. I'm not trying to clone bitops - it's just that I don't have a better idea for the naming.

Bartosz

Mark Brown

maintainer

2020-11-06 12:17 UTC #12 lore raw

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

Quoted 10 lines · Bartosz Golaszewski

Quoted 7 lines · Mark Brown
AFAICT (and indeed now I dig around assign_bit() only works on a single bit and does both shifts which makes the correspondance with that interface super unclear, we're not mirroring that interface here). If you're trying to clone the bitops function it should probably be an actual clone of the bitops function not something different, that would be clearer and it'd be easier to understand why someone would want the API in the first place. But perhaps I'm missing something here?

Quoted 9 lines

It's true that bitops set/clear/assign bit macros work on single bits and take their offsets as arguments. However all regmap helpers operate on masks. Two release cycles back we added two helpers regmap_set_bits() and regmap_clear_bits() which are just wrappers around regmap_update_bits(). The naming was inspired by bitops (because how would one name these operations differently anyway?) but it was supposed to be able to clear/set multiple bits at once - at least this was my use-case in mtk-star-emac driver I was writing at the time and for which I wrote these helpers.

Which is fine and not at all unclear since there's no separate value argument, the value comes along with the name.

Quoted 4 lines

Now the regmap_assign_bits() helper is just an extension to these two which allows users to use one line instead of four. I'm not trying to clone bitops - it's just that I don't have a better idea for the naming.

I really don't see the benefit to the helper, it makes sense in the context of bitops where the operation does all the shifting and it's only a single bit but for regmap where it's dealing with bitmasks as well and the naming doesn't make it crystal clear I can only see this being confusing to people. Had the set and clear helpers for regmap been done as single bits it'd be a lot easier but that's not the case and it'd also be odd to have just this one helper that took a shift rather than a bitmask.

Bartosz Golaszewski

author

2020-11-04 19:31 UTC #13 lore raw

PATCH v2 8/8 gpio: exar: use devm action for freeing the IDA and drop remove()

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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 <[email protected]>

---
 drivers/gpio/gpio-exar.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

drivers/gpio/gpio-exar.c +12 ~~-15~~

index 94ef500567ef..b3372279188d 100644
@@ -108,6 +108,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,

@@ -155,6 +162,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;

@@ -171,29 +182,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,
 	},

Signature · 1 line

2.29.1

Andy Shevchenko

2020-11-04 20:36 UTC #14 lore raw

In reply to Bartosz Golaszewski

Quoted 13 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>
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.
v1 -> v2:
- add new regmap helper: regmap_assign_bits()
- fix lvl vs sel register access
- set value in direction_output callback

Quoted 4 lines

Note: I didn't use the fancy method of offset calculation Andy and David suggested because this series broke the driver according to Jan - let's get it right before we modify it any more. I found a couple problems that could maybe cause the crash. Jan: could you give it another spin?

I would suggest perform two tests:
a) full set
b) with last two patches reverted / not being applied

In that case if the series is still broken we may go with cleanups first.

Quoted 19 lines

Bartosz Golaszewski (8):
regmap: provide regmap_assign_bits()
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 | 151 +++++++++++++++++++--------------------
 include/linux/regmap.h   |  16 +++++
 3 files changed, 90 insertions(+), 78 deletions(-)
--
2.29.1

Signature · 2 lines

With Best Regards,
Andy Shevchenko