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 | 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
On Tue, Nov 10, 2020 at 03:55:51PM +0100, Bartosz Golaszewski wrote:
> 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.
...
> +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
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 <[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.
>
> ...
>
> > +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
On Tue, Nov 10, 2020 at 4:09 PM Andy Shevchenko
<[email protected]> 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 <[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.
> >
> > ...
> >
> > > +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
On Tue, Nov 10, 2020 at 04:12:38PM +0100, Bartosz Golaszewski wrote:
> On Tue, Nov 10, 2020 at 4:09 PM Andy Shevchenko
> <[email protected]> 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 <[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.
> > >
> > > ...
> > >
> > > > +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
On Tue, Nov 10, 2020 at 5:17 PM Andy Shevchenko
<[email protected]> 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
> > <[email protected]> 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 <[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.
> > > >
> > > > ...
> > > >
> > > > > +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
On Tue, Nov 10, 2020 at 6:37 PM Bartosz Golaszewski
<[email protected]> wrote:
> On Tue, Nov 10, 2020 at 5:17 PM Andy Shevchenko
> <[email protected]> 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
On Tue, Nov 10, 2020 at 5:43 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Tue, Nov 10, 2020 at 6:37 PM Bartosz Golaszewski
> <[email protected]> wrote:
> > On Tue, Nov 10, 2020 at 5:17 PM Andy Shevchenko
> > <[email protected]> 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
On Tue, Nov 10, 2020 at 05:52:26PM +0100, Bartosz Golaszewski wrote:
> On Tue, Nov 10, 2020 at 5:43 PM Andy Shevchenko
> <[email protected]> wrote:
> >
> > On Tue, Nov 10, 2020 at 6:37 PM Bartosz Golaszewski
> > <[email protected]> wrote:
> > > On Tue, Nov 10, 2020 at 5:17 PM Andy Shevchenko
> > > <[email protected]> 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