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

[PATCH v4 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.

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

v2 -> v3:
- drop the regmap helper from series

v3 -> v4:
- renamed the regmap variable to 'regmap' as leaving the old name caused me
  to miss an assignment leading to a crash (culprit spotted by Andy Shevchenko)

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 v4 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 v4 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 v4 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 v4 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

[PATCH v4 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..28b0b4b5fa35 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_lvl_addr(exar_gpio, offset);
+	unsigned int bit = exar_offset_to_bit(exar_gpio, offset);
 
 	exar_update(chip, addr, value, bit);
 }
-- 
2.29.1

[PATCH v4 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 28b0b4b5fa35..2fdca872c7c0 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 *regmap;
 	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->regmap, 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->regmap, 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->regmap, addr, BIT(bit));
+	else
+		regmap_clear_bits(exar_gpio->regmap, 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->regmap, 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->regmap, addr, BIT(bit));
+
+	return 0;
 }
 
+static const struct regmap_config exar_regmap_config = {
+	.name		= "exar-gpio",
+	.reg_bits	= 16,
+	.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->regmap = devm_regmap_init_mmio(dev, p, &exar_regmap_config);
+	if (IS_ERR(exar_gpio->regmap))
+		return PTR_ERR(exar_gpio->regmap);
 
 	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;
@@ -181,7 +168,6 @@ static int gpio_exar_probe(struct platform_device *pdev)
 	exar_gpio->gpio_chip.set = exar_set_value;
 	exar_gpio->gpio_chip.base = -1;
 	exar_gpio->gpio_chip.ngpio = ngpios;
-	exar_gpio->regs = p;
 	exar_gpio->index = index;
 	exar_gpio->first_pin = first_pin;
 
@@ -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

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

[Andy Shevchenko]

On Tue, Nov 10, 2020 at 03:55:51PM +0100, Bartosz Golaszewski 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.

...

> +static const struct regmap_config exar_regmap_config = {
> +	.name		= "exar-gpio",
> +	.reg_bits	= 16,

As per previous version comment.

Hold on, the registers are 16-bit wide, but their halves are sparsed!
So, I guess 8 and 8 with helpers to get hi and lo parts are essential.


TABLE 5: DEVICE CONFIGURATION REGISTERS SHOWN IN BYTE ALIGNMENT

> +	.val_bits	= 8,
> +};

This is basically represents two banks out of 6 8-bit registers each.


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Nov 10, 2020 at 05:04:47PM +0200, Andy Shevchenko wrote:
> On Tue, Nov 10, 2020 at 03:55:51PM +0100, Bartosz Golaszewski 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.
> 
> ...
> 
> > +static const struct regmap_config exar_regmap_config = {
> > +	.name		= "exar-gpio",
> > +	.reg_bits	= 16,
> 
> As per previous version comment.
> 
> Hold on, the registers are 16-bit wide, but their halves are sparsed!
> So, I guess 8 and 8 with helpers to get hi and lo parts are essential.
> 
> 
> TABLE 5: DEVICE CONFIGURATION REGISTERS SHOWN IN BYTE ALIGNMENT
> 
> > +	.val_bits	= 8,
> > +};
> 
> This is basically represents two banks out of 6 8-bit registers each.

...which makes me wonder if gpio-regmap can be utilized here...

-- 
With Best Regards,
Andy Shevchenko

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

[Bartosz Golaszewski]

On Tue, Nov 10, 2020 at 4:09 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Nov 10, 2020 at 05:04:47PM +0200, Andy Shevchenko wrote:
> > On Tue, Nov 10, 2020 at 03:55:51PM +0100, Bartosz Golaszewski 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.
> >
> > ...
> >
> > > +static const struct regmap_config exar_regmap_config = {
> > > +   .name           = "exar-gpio",
> > > +   .reg_bits       = 16,
> >
> > As per previous version comment.
> >
> > Hold on, the registers are 16-bit wide, but their halves are sparsed!
> > So, I guess 8 and 8 with helpers to get hi and lo parts are essential.
> >
> >
> > TABLE 5: DEVICE CONFIGURATION REGISTERS SHOWN IN BYTE ALIGNMENT
> >
> > > +   .val_bits       = 8,
> > > +};
> >
> > This is basically represents two banks out of 6 8-bit registers each.
>
> ...which makes me wonder if gpio-regmap can be utilized here...
>

But the address width won't affect the actuall accessing of 8 bits
registers in an mmio regmap. Internally the mmio regmap does pretty
much the same thing the previous driver did: call readb()/writeb() on
8-bit "chunks" of the banks.

Bartosz

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

[Andy Shevchenko]

On Tue, Nov 10, 2020 at 04:12:38PM +0100, Bartosz Golaszewski wrote:
> On Tue, Nov 10, 2020 at 4:09 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Tue, Nov 10, 2020 at 05:04:47PM +0200, Andy Shevchenko wrote:
> > > On Tue, Nov 10, 2020 at 03:55:51PM +0100, Bartosz Golaszewski 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.
> > >
> > > ...
> > >
> > > > +static const struct regmap_config exar_regmap_config = {
> > > > +   .name           = "exar-gpio",
> > > > +   .reg_bits       = 16,
> > >
> > > As per previous version comment.
> > >
> > > Hold on, the registers are 16-bit wide, but their halves are sparsed!
> > > So, I guess 8 and 8 with helpers to get hi and lo parts are essential.
> > >
> > >
> > > TABLE 5: DEVICE CONFIGURATION REGISTERS SHOWN IN BYTE ALIGNMENT
> > >
> > > > +   .val_bits       = 8,
> > > > +};
> > >
> > > This is basically represents two banks out of 6 8-bit registers each.
> >
> > ...which makes me wonder if gpio-regmap can be utilized here...
> >
> 
> But the address width won't affect the actuall accessing of 8 bits
> registers in an mmio regmap. Internally the mmio regmap does pretty
> much the same thing the previous driver did: call readb()/writeb() on
> 8-bit "chunks" of the banks.

It will affect reg dump in debugfs. I would really narrow down the register
address space in the config, otherwise that debugfs facility will screw up a
lot of things.

So, and to be on pedantic side...

"The Device Configuration Registers and the two individual UART Configuration
Registers of the XR17V352 occupy 2K of PCI bus memory address space."

11 seems the correct value for the address width.

-- 
With Best Regards,
Andy Shevchenko

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

[Bartosz Golaszewski]

On Tue, Nov 10, 2020 at 5:17 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Nov 10, 2020 at 04:12:38PM +0100, Bartosz Golaszewski wrote:
> > On Tue, Nov 10, 2020 at 4:09 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Tue, Nov 10, 2020 at 05:04:47PM +0200, Andy Shevchenko wrote:
> > > > On Tue, Nov 10, 2020 at 03:55:51PM +0100, Bartosz Golaszewski 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.
> > > >
> > > > ...
> > > >
> > > > > +static const struct regmap_config exar_regmap_config = {
> > > > > +   .name           = "exar-gpio",
> > > > > +   .reg_bits       = 16,
> > > >
> > > > As per previous version comment.
> > > >
> > > > Hold on, the registers are 16-bit wide, but their halves are sparsed!
> > > > So, I guess 8 and 8 with helpers to get hi and lo parts are essential.
> > > >
> > > >
> > > > TABLE 5: DEVICE CONFIGURATION REGISTERS SHOWN IN BYTE ALIGNMENT
> > > >
> > > > > +   .val_bits       = 8,
> > > > > +};
> > > >
> > > > This is basically represents two banks out of 6 8-bit registers each.
> > >
> > > ...which makes me wonder if gpio-regmap can be utilized here...
> > >
> >
> > But the address width won't affect the actuall accessing of 8 bits
> > registers in an mmio regmap. Internally the mmio regmap does pretty
> > much the same thing the previous driver did: call readb()/writeb() on
> > 8-bit "chunks" of the banks.
>
> It will affect reg dump in debugfs. I would really narrow down the register
> address space in the config, otherwise that debugfs facility will screw up a
> lot of things.
>
> So, and to be on pedantic side...
>
> "The Device Configuration Registers and the two individual UART Configuration
> Registers of the XR17V352 occupy 2K of PCI bus memory address space."
>
> 11 seems the correct value for the address width.
>

I take it as a typo and assume you meant 16. So the patch should be
correct and your review tag is good to go?

Bartosz

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

[Andy Shevchenko]

On Tue, Nov 10, 2020 at 6:37 PM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
> On Tue, Nov 10, 2020 at 5:17 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

...

> > > > > > +static const struct regmap_config exar_regmap_config = {
> > > > > > +   .name           = "exar-gpio",
> > > > > > +   .reg_bits       = 16,
> > > > >
> > > > > As per previous version comment.
> > > > >
> > > > > Hold on, the registers are 16-bit wide, but their halves are sparsed!
> > > > > So, I guess 8 and 8 with helpers to get hi and lo parts are essential.
> > > > >
> > > > >
> > > > > TABLE 5: DEVICE CONFIGURATION REGISTERS SHOWN IN BYTE ALIGNMENT
> > > > >
> > > > > > +   .val_bits       = 8,
> > > > > > +};
> > > > >
> > > > > This is basically represents two banks out of 6 8-bit registers each.
> > > >
> > > > ...which makes me wonder if gpio-regmap can be utilized here...
> > > >
> > >
> > > But the address width won't affect the actuall accessing of 8 bits
> > > registers in an mmio regmap. Internally the mmio regmap does pretty
> > > much the same thing the previous driver did: call readb()/writeb() on
> > > 8-bit "chunks" of the banks.
> >
> > It will affect reg dump in debugfs. I would really narrow down the register
> > address space in the config, otherwise that debugfs facility will screw up a
> > lot of things.
> >
> > So, and to be on pedantic side...
> >
> > "The Device Configuration Registers and the two individual UART Configuration
> > Registers of the XR17V352 occupy 2K of PCI bus memory address space."
> >
> > 11 seems the correct value for the address width.
>
> I take it as a typo and assume you meant 16. So the patch should be
> correct and your review tag is good to go?

It's not a typo. But thinking again. This is basically done in regmap
to support serial buses. Here we have MMIO pretty much with 32-bit or
64-bit address accesses. I didn't dig into regmap implementation to
understand the consequences of changing this to the different values
(it seems like rather offset, and in this case 11 is a correct one,
not a typo, and regmap is okay with that).
But I would rather ask Jan to actually mount debugfs and dump
registers and see if it screws up the UART (because it may go all over
important registers), that's why I think this configuration is still
missing some strict rules about what addresses (offsets) driver may or
may not access.



-- 
With Best Regards,
Andy Shevchenko

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

[Bartosz Golaszewski]

On Tue, Nov 10, 2020 at 5:43 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Tue, Nov 10, 2020 at 6:37 PM Bartosz Golaszewski
> <bgolaszewski@baylibre.com> wrote:
> > On Tue, Nov 10, 2020 at 5:17 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
>
> ...
>
> > > > > > > +static const struct regmap_config exar_regmap_config = {
> > > > > > > +   .name           = "exar-gpio",
> > > > > > > +   .reg_bits       = 16,
> > > > > >
> > > > > > As per previous version comment.
> > > > > >
> > > > > > Hold on, the registers are 16-bit wide, but their halves are sparsed!
> > > > > > So, I guess 8 and 8 with helpers to get hi and lo parts are essential.
> > > > > >
> > > > > >
> > > > > > TABLE 5: DEVICE CONFIGURATION REGISTERS SHOWN IN BYTE ALIGNMENT
> > > > > >
> > > > > > > +   .val_bits       = 8,
> > > > > > > +};
> > > > > >
> > > > > > This is basically represents two banks out of 6 8-bit registers each.
> > > > >
> > > > > ...which makes me wonder if gpio-regmap can be utilized here...
> > > > >
> > > >
> > > > But the address width won't affect the actuall accessing of 8 bits
> > > > registers in an mmio regmap. Internally the mmio regmap does pretty
> > > > much the same thing the previous driver did: call readb()/writeb() on
> > > > 8-bit "chunks" of the banks.
> > >
> > > It will affect reg dump in debugfs. I would really narrow down the register
> > > address space in the config, otherwise that debugfs facility will screw up a
> > > lot of things.
> > >
> > > So, and to be on pedantic side...
> > >
> > > "The Device Configuration Registers and the two individual UART Configuration
> > > Registers of the XR17V352 occupy 2K of PCI bus memory address space."
> > >
> > > 11 seems the correct value for the address width.
> >
> > I take it as a typo and assume you meant 16. So the patch should be
> > correct and your review tag is good to go?
>
> It's not a typo. But thinking again. This is basically done in regmap
> to support serial buses. Here we have MMIO pretty much with 32-bit or
> 64-bit address accesses. I didn't dig into regmap implementation to
> understand the consequences of changing this to the different values
> (it seems like rather offset, and in this case 11 is a correct one,
> not a typo, and regmap is okay with that).
> But I would rather ask Jan to actually mount debugfs and dump
> registers and see if it screws up the UART (because it may go all over
> important registers), that's why I think this configuration is still
> missing some strict rules about what addresses (offsets) driver may or
> may not access.

Ok now I get it. Yes 11 seems to be right in this case for the max
address. We can implement the readable/writable callbacks to be very
strict about the register accesses but isn't it overkill? This driver
is very small and only accesses a couple registers. I don't see such
strict checking very often except for very complicated modules (like
pca953x you mentioned).

Bartosz

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

[Andy Shevchenko]

On Tue, Nov 10, 2020 at 05:52:26PM +0100, Bartosz Golaszewski wrote:
> On Tue, Nov 10, 2020 at 5:43 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Tue, Nov 10, 2020 at 6:37 PM Bartosz Golaszewski
> > <bgolaszewski@baylibre.com> wrote:
> > > On Tue, Nov 10, 2020 at 5:17 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:

...

> > It's not a typo. But thinking again. This is basically done in regmap
> > to support serial buses. Here we have MMIO pretty much with 32-bit or
> > 64-bit address accesses. I didn't dig into regmap implementation to
> > understand the consequences of changing this to the different values
> > (it seems like rather offset, and in this case 11 is a correct one,
> > not a typo, and regmap is okay with that).
> > But I would rather ask Jan to actually mount debugfs and dump
> > registers and see if it screws up the UART (because it may go all over
> > important registers), that's why I think this configuration is still
> > missing some strict rules about what addresses (offsets) driver may or
> > may not access.
> 
> Ok now I get it. Yes 11 seems to be right in this case for the max
> address. We can implement the readable/writable callbacks to be very
> strict about the register accesses but isn't it overkill? This driver
> is very small and only accesses a couple registers. I don't see such
> strict checking very often except for very complicated modules (like
> pca953x you mentioned).

Maybe a comment in commit message or code that this has no protection against
access to out of boundary registers.

Keep my tag after choosing 11 and whatever you decide for access to non-GPIO
registers from this driver. I'm not blocking this from upstreaming since we
have got a confirmation that main functionality works as expected.


-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 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 2fdca872c7c0..d37de78247a6 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -111,6 +111,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	= 16,
@@ -158,6 +165,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 v4 0/7] gpio: exar: refactor the driver

[Andy Shevchenko]

On Tue, Nov 10, 2020 at 03:55:45PM +0100, Bartosz Golaszewski 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.

Thanks.
With reverted reg_width change
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> v1 -> v2:
> - add new regmap helper: regmap_assign_bits()
> - fix lvl vs sel register access
> - set value in direction_output callback
> 
> v2 -> v3:
> - drop the regmap helper from series
> 
> v3 -> v4:
> - renamed the regmap variable to 'regmap' as leaving the old name caused me
>   to miss an assignment leading to a crash (culprit spotted by Andy Shevchenko)
> 
> 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: [PATCH v4 0/7] gpio: exar: refactor the driver

[Andy Shevchenko]

On Tue, Nov 10, 2020 at 5:07 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Tue, Nov 10, 2020 at 03:55:45PM +0100, Bartosz Golaszewski wrote:

> With reverted reg_width change

I should have relaxed this to "with whatever settlement we become
about regmap configuration".

> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>


-- 
With Best Regards,
Andy Shevchenko