gpio: exar: refactor the driver

From: Bartosz Golaszewski <[email protected]>

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

Quoted 5 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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

...

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

Quoted 3 lines

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

+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

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

Quoted 2 lines

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

+	.val_bits	= 8,
+};

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

Quoted 19 lines · Andy Shevchenko

Quoted 5 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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

...

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

Quoted 3 lines

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

+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.

Quoted 6 lines

TABLE 5: DEVICE CONFIGURATION REGISTERS SHOWN IN BYTE ALIGNMENT

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

Quoted 2 lines

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

+   .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...

Quoted 28 lines · Andy Shevchenko

Quoted 25 lines · Andy Shevchenko

Quoted 5 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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

...

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

Quoted 3 lines

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

+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

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

Quoted 2 lines

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

+   .val_bits       = 8,
+};

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

Quoted 2 lines

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

Quoted 30 lines · Bartosz Golaszewski

Quoted 28 lines · Andy Shevchenko

Quoted 25 lines · Andy Shevchenko

Quoted 5 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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

...

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

Quoted 3 lines

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

+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

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

Quoted 2 lines

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

+   .val_bits       = 8,
+};

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

Quoted 7 lines

Quoted 2 lines

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

On Tue, Nov 10, 2020 at 5:17 PM Andy Shevchenko
<[email protected]> wrote:

Quoted 31 lines

Quoted 17 lines

Quoted 17 lines

Quoted 16 lines

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

Quoted 3 lines

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

+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

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

Quoted 2 lines

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

+   .val_bits       = 8,
+};

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

Quoted 7 lines

Quoted 2 lines

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

Quoted 4 lines

"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?

Quoted 2 lines · Bartosz Golaszewski

On Tue, Nov 10, 2020 at 5:17 PM Andy Shevchenko
<[email protected]> wrote:

...

Quoted 31 lines

Quoted 31 lines

Quoted 17 lines

Quoted 17 lines

Quoted 16 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+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

Review

Quoted 2 lines

(quoted diff) +2 -0

+   .val_bits       = 8,
+};

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

Quoted 7 lines

Quoted 2 lines

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

Quoted 7 lines

Quoted 4 lines

"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.

Quoted 5 lines · Andy Shevchenko

Quoted 2 lines · Bartosz Golaszewski

On Tue, Nov 10, 2020 at 5:17 PM Andy Shevchenko
<[email protected]> wrote:

Quoted 11 lines

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).

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

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(-)

Signature · 1 line

2.29.1

On Tue, Nov 10, 2020 at 03:55:45PM +0100, Bartosz Golaszewski wrote:

With reverted reg_width change

Reviewed-by Andy Shevchenko <[email protected]>