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

From: Jan Kiszka
Date: Tue Nov 10 2020 - 08:46:43 EST

Next message: Will Deacon: "Re: [PATCH v19 0/4] iommu/arm-smmu: Add adreno-smmu implementation and bindings"
Previous message: Daniel Vetter: "Re: [PATCH 1/2] console: Remove dummy con_font_op() callback implementations"
In reply to: Bartosz Golaszewski: "[PATCH v3 6/7] gpio: exar: switch to using regmap"
Next in thread: Bartosz Golaszewski: "Re: [PATCH v3 6/7] gpio: exar: switch to using regmap"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 10.11.20 13:34, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
> ---
> drivers/gpio/Kconfig | 1 +
> drivers/gpio/gpio-exar.c | 90 +++++++++++++++++-----------------------
> 2 files changed, 38 insertions(+), 53 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..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;
> }
>

Unfortunately, this one still crashes:

...
0000:02:00.0: ttyS2 at MMIO 0x90000000 (irq = 44, base_baud = 7812500) is a XR17V35X
0000:02:00.0: ttyS3 at MMIO 0x90000400 (irq = 44, base_baud = 7812500) is a XR17V35X
BUG: kernel NULL pointer dereference, address: 00000000
#PF: supervisor instruction fetch in kernel mode
#PF: error_code(0xc3300010) - not-present page
*pde = 00000000
Oops: 0010 [#1] PREEMPT
CPU: 0 PID: 5 Comm: kworker/0:0 Not tainted 5.10.0-rc2+ #440
Hardware name: Intel Corp. QUARK/SIMATIC IOT2000, BIOS V24.02.01 10/30/2018
Workqueue: events deferred_probe_work_func
EIP: 0x0
Code: Unable to access opcode bytes at RIP 0xffffffd6.
EAX: 00000000 EBX: f7c74000 ECX: 00000004 EDX: 00000096
ESI: 00000000 EDI: 00000000 EBP: c1157d88 ESP: c1157d70
DS: 007b ES: 007b FS: 0000 GS: 00e0 SS: 0068 EFLAGS: 00010286
CR0: 80050033 CR2: ffffffd6 CR3: 03778000 CR4: 00100010
Call Trace:
regmap_update_bits_base+0x22/0x60
exar_set_value+0x4e/0x70 [gpio_exar]
exar_direction_output+0x3a/0x80 [gpio_exar]
? exar_set_value+0x70/0x70 [gpio_exar]
? exar_set_value+0x70/0x70 [gpio_exar]
gpiod_direction_output_raw_commit+0x74/0x270
? exar_direction_input+0x50/0x50 [gpio_exar]
? exar_set_value+0x70/0x70 [gpio_exar]
gpiod_direction_output+0xf0/0x160
create_gpio_led+0xea/0x180
gpio_led_probe+0x22c/0x460
? device_pm_check_callbacks+0x4c/0x100
platform_drv_probe+0x2d/0x80
really_probe+0xcb/0x330
driver_probe_device+0x49/0xa0
__device_attach_driver+0x61/0x80
? driver_allows_async_probing+0x60/0x60
bus_for_each_drv+0x4f/0x90
__device_attach+0xbb/0x120
? driver_allows_async_probing+0x60/0x60
device_initial_probe+0x12/0x20
bus_probe_device+0x6f/0x80
deferred_probe_work_func+0x56/0x80
process_one_work+0x1ce/0x390
worker_thread+0x37/0x420
kthread+0x115/0x130
? process_one_work+0x390/0x390
? kthread_create_on_node+0x20/0x20
ret_from_fork+0x19/0x30
Modules linked in: gpio_exar(+) spi_pxa2xx_platform(+) 8250_exar spi_pxa2xx_pci ti_adc108s102 industrialio_triggered_buffer kfifo_buf industrialio at24
CR2: 0000000000000000
---[ end trace 8ea7faca198463b8 ]---
EIP: 0x0
Code: Unable to access opcode bytes at RIP 0xffffffd6.
EAX: 00000000 EBX: f7c74000 ECX: 00000004 EDX: 00000096
ESI: 00000000 EDI: 00000000 EBP: c1157d88 ESP: c1157d70
DS: 007b ES: 007b FS: 0000 GS: 00e0 SS: 0068 EFLAGS: 00010286
CR0: 80050033 CR2: ffffffd6 CR3: 03778000 CR4: 00100010

Jan

Next message: Will Deacon: "Re: [PATCH v19 0/4] iommu/arm-smmu: Add adreno-smmu implementation and bindings"
Previous message: Daniel Vetter: "Re: [PATCH 1/2] console: Remove dummy con_font_op() callback implementations"
In reply to: Bartosz Golaszewski: "[PATCH v3 6/7] gpio: exar: switch to using regmap"
Next in thread: Bartosz Golaszewski: "Re: [PATCH v3 6/7] gpio: exar: switch to using regmap"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]