| Message ID | 20201110123406.3261-7-brgl@bgdev.pl |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/7] gpio: exar: add a newline after the copyright notice | expand |
On Tue, Nov 10, 2020 at 2:23 PM Jan Kiszka <jan.kiszka@siemens.com> wrote: > > > Unfortunately, this one still crashes: > Just to confirm: does patch 5/7 alone work? Bartosz
On Tue, Nov 10, 2020 at 2:35 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: ... > struct exar_gpio_chip { > struct gpio_chip gpio_chip; > - struct mutex lock; > + struct regmap *regs; Leaving the same name is a call for potential troubles. > int index; > - void __iomem *regs; > char name[20]; > unsigned int first_pin; > }; ... > +static const struct regmap_config exar_regmap_config = { > + .name = "exar-gpio", > + .reg_bits = 8, > + .val_bits = 8, > +}; Looking at the crash, are you sure this is a comprehensive description? Maybe it requires something like stride or so? -- With Best Regards, Andy Shevchenko
On Tue, Nov 10, 2020 at 3:11 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Tue, Nov 10, 2020 at 2:35 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > ... > > > struct exar_gpio_chip { > > struct gpio_chip gpio_chip; > > - struct mutex lock; > > > + struct regmap *regs; > > Leaving the same name is a call for potential troubles. > > > int index; > > - void __iomem *regs; > > char name[20]; > > unsigned int first_pin; > > }; > > ... > > > +static const struct regmap_config exar_regmap_config = { > > + .name = "exar-gpio", > > + .reg_bits = 8, > > + .val_bits = 8, > > +}; > > Looking at the crash, are you sure this is a comprehensive description? > Maybe it requires something like stride or so? > This is what I'm looking at ATM. Looking at the datasheet[1], there are no breaks in the registers so the default stride of 1 should be fine as is the value bits width of 8. I think that I got the address width wrong though. Should be 16 bits probably. Jan: could you change reg_bits to 16 and try again? Bartosz [1] https://www.maxlinear.com/ds/xr17v352.pdf
On Tue, Nov 10, 2020 at 01:34:05PM +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. ... > + /* > + * We don't need to check the return values of mmio regmap operations (unless > + * the regmap has a clock attached which is not the case here). > + */ > + exar_gpio->regs = devm_regmap_init_mmio(dev, p, &exar_regmap_config); > + if (IS_ERR(exar_gpio->regs)) > + return PTR_ERR(exar_gpio->regs); > > index = ida_alloc(&ida_index, GFP_KERNEL); > - if (index < 0) { > - ret = index; > - goto err_mutex_destroy; > - } > + if (index < 0) > + return index; And below you effectively use p as regmap! That's what renaming of variable regs -> regmap or map can easily reveal. exar_gpio->regs = p; -- With Best Regards, Andy Shevchenko
On Tue, Nov 10, 2020 at 04:26:24PM +0200, Andy Shevchenko wrote: > On Tue, Nov 10, 2020 at 01:34:05PM +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. > > ... > > > + /* > > + * We don't need to check the return values of mmio regmap operations (unless > > + * the regmap has a clock attached which is not the case here). > > + */ > > + exar_gpio->regs = devm_regmap_init_mmio(dev, p, &exar_regmap_config); > > + if (IS_ERR(exar_gpio->regs)) > > + return PTR_ERR(exar_gpio->regs); > > > > index = ida_alloc(&ida_index, GFP_KERNEL); > > - if (index < 0) { > > - ret = index; > > - goto err_mutex_destroy; > > - } > > + if (index < 0) > > + return index; > > And below you effectively use p as regmap! > That's what renaming of variable regs -> regmap or map can easily reveal. > > exar_gpio->regs = p; Jan, if you remove this line, does it help? -- With Best Regards, Andy Shevchenko
On Tue, Nov 10, 2020 at 3:26 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Nov 10, 2020 at 04:26:24PM +0200, Andy Shevchenko wrote: > > On Tue, Nov 10, 2020 at 01:34:05PM +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. > > > > ... > > > > > + /* > > > + * We don't need to check the return values of mmio regmap operations (unless > > > + * the regmap has a clock attached which is not the case here). > > > + */ > > > + exar_gpio->regs = devm_regmap_init_mmio(dev, p, &exar_regmap_config); > > > + if (IS_ERR(exar_gpio->regs)) > > > + return PTR_ERR(exar_gpio->regs); > > > > > > index = ida_alloc(&ida_index, GFP_KERNEL); > > > - if (index < 0) { > > > - ret = index; > > > - goto err_mutex_destroy; > > > - } > > > + if (index < 0) > > > + return index; > > > > And below you effectively use p as regmap! > > That's what renaming of variable regs -> regmap or map can easily reveal. > > > > exar_gpio->regs = p; > > Jan, if you remove this line, does it help? > Ha! I guess you were right saying that keeping the name is asking for trouble then. :) I think that may be it but address width should still be changed to 16. Bartosz
On 10.11.20 15:30, Bartosz Golaszewski wrote: > On Tue, Nov 10, 2020 at 3:26 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: >> >> On Tue, Nov 10, 2020 at 04:26:24PM +0200, Andy Shevchenko wrote: >>> On Tue, Nov 10, 2020 at 01:34:05PM +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. >>> >>> ... >>> >>>> + /* >>>> + * We don't need to check the return values of mmio regmap operations (unless >>>> + * the regmap has a clock attached which is not the case here). >>>> + */ >>>> + exar_gpio->regs = devm_regmap_init_mmio(dev, p, &exar_regmap_config); >>>> + if (IS_ERR(exar_gpio->regs)) >>>> + return PTR_ERR(exar_gpio->regs); >>>> >>>> index = ida_alloc(&ida_index, GFP_KERNEL); >>>> - if (index < 0) { >>>> - ret = index; >>>> - goto err_mutex_destroy; >>>> - } >>>> + if (index < 0) >>>> + return index; >>> >>> And below you effectively use p as regmap! >>> That's what renaming of variable regs -> regmap or map can easily reveal. >>> >>> exar_gpio->regs = p; >> >> Jan, if you remove this line, does it help? >> > > Ha! I guess you were right saying that keeping the name is asking for > trouble then. :) > > I think that may be it but address width should still be changed to 16. > Removing the line that Andy found made things work here. And switching to 16 for reg_bits didn't make things worse again. Jan -- Siemens AG, T RDA IOT Corporate Competence Center Embedded Linux
On Tue, Nov 10, 2020 at 3:50 PM Jan Kiszka <jan.kiszka@siemens.com> wrote: > > > On 10.11.20 15:30, Bartosz Golaszewski wrote: > > On Tue, Nov 10, 2020 at 3:26 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > >> > >> On Tue, Nov 10, 2020 at 04:26:24PM +0200, Andy Shevchenko wrote: > >>> On Tue, Nov 10, 2020 at 01:34:05PM +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. > >>> > >>> ... > >>> > >>>> + /* > >>>> + * We don't need to check the return values of mmio regmap operations (unless > >>>> + * the regmap has a clock attached which is not the case here). > >>>> + */ > >>>> + exar_gpio->regs = devm_regmap_init_mmio(dev, p, &exar_regmap_config); > >>>> + if (IS_ERR(exar_gpio->regs)) > >>>> + return PTR_ERR(exar_gpio->regs); > >>>> > >>>> index = ida_alloc(&ida_index, GFP_KERNEL); > >>>> - if (index < 0) { > >>>> - ret = index; > >>>> - goto err_mutex_destroy; > >>>> - } > >>>> + if (index < 0) > >>>> + return index; > >>> > >>> And below you effectively use p as regmap! > >>> That's what renaming of variable regs -> regmap or map can easily reveal. > >>> > >>> exar_gpio->regs = p; > >> > >> Jan, if you remove this line, does it help? > >> > > > > Ha! I guess you were right saying that keeping the name is asking for > > trouble then. :) > > > > I think that may be it but address width should still be changed to 16. > > > > Removing the line that Andy found made things work here. And switching > to 16 for reg_bits didn't make things worse again. > > Jan Alright! I'll send a v4 with these things fixed then. Bartosz
On Tue, Nov 10, 2020 at 03:52:00PM +0100, Bartosz Golaszewski wrote: > On Tue, Nov 10, 2020 at 3:50 PM Jan Kiszka <jan.kiszka@siemens.com> wrote: > > On 10.11.20 15:30, Bartosz Golaszewski wrote: ... > > Removing the line that Andy found made things work here. And switching > > to 16 for reg_bits didn't make things worse again. > > > > Jan > > Alright! I'll send a v4 with these things fixed then. 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. -- With Best Regards, Andy Shevchenko
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..a2d324c513f8 100644 --- a/drivers/gpio/gpio-exar.c +++ b/drivers/gpio/gpio-exar.c @@ -14,6 +14,7 @@ #include <linux/module.h> #include <linux/pci.h> #include <linux/platform_device.h> +#include <linux/regmap.h> #define EXAR_OFFSET_MPIOLVL_LO 0x90 #define EXAR_OFFSET_MPIOSEL_LO 0x93 @@ -26,9 +27,8 @@ static DEFINE_IDA(ida_index); struct exar_gpio_chip { struct gpio_chip gpio_chip; - struct mutex lock; + struct regmap *regs; int index; - void __iomem *regs; char name[20]; unsigned int first_pin; }; @@ -53,51 +53,13 @@ exar_offset_to_bit(struct exar_gpio_chip *exar_gpio, unsigned int offset) return (offset + exar_gpio->first_pin) % 8; } -static void exar_update(struct gpio_chip *chip, unsigned int reg, int val, - unsigned int offset) -{ - struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip); - int temp; - - mutex_lock(&exar_gpio->lock); - temp = readb(exar_gpio->regs + reg); - temp &= ~BIT(offset); - if (val) - temp |= BIT(offset); - writeb(temp, exar_gpio->regs + reg); - mutex_unlock(&exar_gpio->lock); -} - -static int exar_set_direction(struct gpio_chip *chip, int direction, - unsigned int offset) -{ - struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip); - unsigned int addr = exar_offset_to_sel_addr(exar_gpio, offset); - unsigned int bit = exar_offset_to_bit(exar_gpio, offset); - - exar_update(chip, addr, direction, bit); - return 0; -} - -static int exar_get(struct gpio_chip *chip, unsigned int reg) -{ - struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip); - int value; - - mutex_lock(&exar_gpio->lock); - value = readb(exar_gpio->regs + reg); - mutex_unlock(&exar_gpio->lock); - - return value; -} - static int exar_get_direction(struct gpio_chip *chip, unsigned int offset) { struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip); unsigned int addr = exar_offset_to_sel_addr(exar_gpio, offset); unsigned int bit = exar_offset_to_bit(exar_gpio, offset); - if (exar_get(chip, addr) & BIT(bit)) + if (regmap_test_bits(exar_gpio->regs, addr, BIT(bit))) return GPIO_LINE_DIRECTION_IN; return GPIO_LINE_DIRECTION_OUT; @@ -109,7 +71,7 @@ static int exar_get_value(struct gpio_chip *chip, unsigned int offset) unsigned int addr = exar_offset_to_lvl_addr(exar_gpio, offset); unsigned int bit = exar_offset_to_bit(exar_gpio, offset); - return !!(exar_get(chip, addr) & BIT(bit)); + return !!(regmap_test_bits(exar_gpio->regs, addr, BIT(bit))); } static void exar_set_value(struct gpio_chip *chip, unsigned int offset, @@ -119,21 +81,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->regs, addr, BIT(bit)); + else + regmap_clear_bits(exar_gpio->regs, addr, BIT(bit)); } static int exar_direction_output(struct gpio_chip *chip, unsigned int offset, int value) { + 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->regs, addr, BIT(bit)); + + return 0; } static int exar_direction_input(struct gpio_chip *chip, unsigned int offset) { - return exar_set_direction(chip, 1, offset); + struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip); + unsigned int addr = exar_offset_to_sel_addr(exar_gpio, offset); + unsigned int bit = exar_offset_to_bit(exar_gpio, offset); + + regmap_set_bits(exar_gpio->regs, addr, BIT(bit)); + + return 0; } +static const struct regmap_config exar_regmap_config = { + .name = "exar-gpio", + .reg_bits = 8, + .val_bits = 8, +}; + static int gpio_exar_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -163,13 +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->regs = devm_regmap_init_mmio(dev, p, &exar_regmap_config); + if (IS_ERR(exar_gpio->regs)) + return PTR_ERR(exar_gpio->regs); index = ida_alloc(&ida_index, GFP_KERNEL); - if (index < 0) { - ret = index; - goto err_mutex_destroy; - } + if (index < 0) + return index; sprintf(exar_gpio->name, "exar_gpio%d", index); exar_gpio->gpio_chip.label = exar_gpio->name; @@ -195,8 +182,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 +190,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; }