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.
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
From: Bartosz Golaszewski <[email protected]>
It's customary to have a newline between the copyright header and the
includes. Add one to gpio-exar.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
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 <[email protected]>
*/
+
#include <linux/bitops.h>
#include <linux/device.h>
#include <linux/gpio/driver.h>
--
2.29.1
From: Bartosz Golaszewski <[email protected]>
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 <[email protected]>
---
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
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 629f4dad6919..e4da278f74a2 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,41 @@ static void exar_set_value(struct gpio_chip *chip, unsigned int offset,
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, 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)
{
- exar_set_value(chip, offset, value);
- return exar_set_direction(chip, 0, 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_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 +145,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 +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 Mon, Oct 26, 2020 at 4:22 PM Bartosz Golaszewski <[email protected]> wrote:
>
> 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.
+Cc: Jan, AFAIR their devices are using Exar UART.
> 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
On 26.10.20 15:46, Andy Shevchenko wrote:
> On Mon, Oct 26, 2020 at 4:22 PM Bartosz Golaszewski <[email protected]> wrote:
>>
>> 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.
>
> +Cc: Jan, AFAIR their devices are using Exar UART.
>
Thanks for CC'ing. I cannot promise testing this soon, but I will try my
best.
Jan
>> 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
>>
>
--
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux
On 27.10.20 16:12, Jan Kiszka wrote:
> On 26.10.20 15:46, Andy Shevchenko wrote:
>> On Mon, Oct 26, 2020 at 4:22 PM Bartosz Golaszewski <[email protected]> wrote:
>>>
>>> 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.
>>
>> +Cc: Jan, AFAIR their devices are using Exar UART.
>>
>
> Thanks for CC'ing. I cannot promise testing this soon, but I will try my
> best.
>
Finally tested, unfortunately with bad news:
...
at24 i2c-INT3499:00: 1024 byte INT3499:00 EEPROM, writable, 1 bytes/write
pxa2xx_spi_pci 0000:00:15.0: enabling device (0000 -> 0002)
pxa2xx_spi_pci 0000:00:15.1: enabling device (0000 -> 0002)
exar_serial 0000:02:00.0: enabling device (0000 -> 0002)
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(0xc1150010) - not-present page
*pde = 00000000
Oops: 0010 [#1] PREEMPT
CPU: 0 PID: 5 Comm: kworker/0:0 Not tainted 5.10.0-rc2+ #438
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: 00000099
ESI: 00000000 EDI: 00000000 EBP: c1157da8 ESP: c1157d90
DS: 007b ES: 007b FS: 0000 GS: 00e0 SS: 0068 EFLAGS: 00010282
CR0: 80050033 CR2: ffffffd6 CR3: 03771000 CR4: 00100010
Call Trace:
regmap_update_bits_base+0x22/0x60
? exar_set_value+0x70/0x70 [gpio_exar]
? exar_set_value+0x70/0x70 [gpio_exar]
exar_direction_output+0x47/0x50 [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 d982fb210f759304 ]---
EIP: 0x0
Code: Unable to access opcode bytes at RIP 0xffffffd6.
EAX: 00000000 EBX: f7c74000 ECX: 00000004 EDX: 00000099
ESI: 00000000 EDI: 00000000 EBP: c1157da8 ESP: c1157d90
DS: 007b ES: 007b FS: 0000 GS: 00e0 SS: 0068 EFLAGS: 00010282
CR0: 80050033 CR2: ffffffd6 CR3: 03771000 CR4: 00100010
Jan
--
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux
On Wed, Nov 4, 2020 at 3:57 PM Jan Kiszka <[email protected]> wrote:
> On 27.10.20 16:12, Jan Kiszka wrote:
> > On 26.10.20 15:46, Andy Shevchenko wrote:
> >> On Mon, Oct 26, 2020 at 4:22 PM Bartosz Golaszewski <[email protected]> wrote:
> >>>
> >>> 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.
> >>
> >> +Cc: Jan, AFAIR their devices are using Exar UART.
> >>
> >
> > Thanks for CC'ing. I cannot promise testing this soon, but I will try my
> > best.
> >
>
> Finally tested, unfortunately with bad news:
> Code: Unable to access opcode bytes at RIP 0xffffffd6.
I guess it is due to missed error pointer handling somewhere, as above
is equal to -ENOMSG.
--
With Best Regards,
Andy Shevchenko
On Wed, Nov 4, 2020 at 3:51 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Wed, Nov 4, 2020 at 3:57 PM Jan Kiszka <[email protected]> wrote:
> > On 27.10.20 16:12, Jan Kiszka wrote:
> > > On 26.10.20 15:46, Andy Shevchenko wrote:
> > >> On Mon, Oct 26, 2020 at 4:22 PM Bartosz Golaszewski <[email protected]> wrote:
> > >>>
> > >>> 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.
> > >>
> > >> +Cc: Jan, AFAIR their devices are using Exar UART.
> > >>
> > >
> > > Thanks for CC'ing. I cannot promise testing this soon, but I will try my
> > > best.
> > >
> >
> > Finally tested, unfortunately with bad news:
>
> > Code: Unable to access opcode bytes at RIP 0xffffffd6.
>
> I guess it is due to missed error pointer handling somewhere, as above
> is equal to -ENOMSG.
>
Yeah I'd guess it's the regmap pointer but we do check the return
value of regmap init with IS_ERR(). :/
Bartosz
On Mon, Oct 26, 2020 at 3:18 PM Bartosz Golaszewski <[email protected]> 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.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
[snip]
>
> static int exar_direction_output(struct gpio_chip *chip, unsigned int offset,
> int value)
> {
> - exar_set_value(chip, offset, value);
> - return exar_set_direction(chip, 0, 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_clear_bits(exar_gpio->regs, addr, BIT(bit));
> +
> + return 0;
> }
>
Upon closer look I noticed this now ignores the value argument. I
doubt however it's the culprit of the crash Jan reported.
[snip]
Bartosz