| Message ID | 20201026141839.28536-1-brgl@bgdev.pl |
|---|---|
| Headers | show |
| Series | gpio: exar: refactor the driver | expand |
On Mon, Oct 26, 2020 at 4:22 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > 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 Mon, Oct 26, 2020 at 4:23 PM Bartosz Golaszewski <brgl@bgdev.pl> 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. ... > + if (value) > + regmap_set_bits(exar_gpio->regs, addr, BIT(bit)); > + else > + regmap_clear_bits(exar_gpio->regs, addr, BIT(bit)); A side note: perhaps + regmap_assign_bits() ? -- With Best Regards, Andy Shevchenko
On Mon, Oct 26, 2020 at 4:22 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > We can simplify the error path in probe() and drop remove() entirely if > we provide a devm action for freeing the device ID. Always the same question to IDR/IDA users: does it guarantee that when the driver is gone the IDR/IDA resources are freed? (It's not directly related to this patch, though)
On Mon, Oct 26, 2020 at 4:46 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Oct 26, 2020 at 4:22 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > 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. From code perspective looks good to me, FWIW, Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> (One nit to one patch, but I think it should be fine) > > 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() -- With Best Regards, Andy Shevchenko
On Mon, Oct 26, 2020 at 4:02 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Oct 26, 2020 at 4:22 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > We can simplify the error path in probe() and drop remove() entirely if > > we provide a devm action for freeing the device ID. > > Always the same question to IDR/IDA users: > does it guarantee that when the driver is gone the IDR/IDA resources are freed? > > (It's not directly related to this patch, though) > Yes because there's exactly one ida_free(id) per id = ida_alloc() here. Bartosz
On 26.10.20 15:46, Andy Shevchenko wrote: > On Mon, Oct 26, 2020 at 4:22 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: >> >> From: Bartosz Golaszewski <bgolaszewski@baylibre.com> >> >> 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 >> >
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 <brgl@bgdev.pl> wrote: >>> >>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com> >>> >>> 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
On Wed, Nov 4, 2020 at 3:57 PM Jan Kiszka <jan.kiszka@siemens.com> 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 <brgl@bgdev.pl> wrote: > >>> > >>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > >>> > >>> 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.
On Wed, Nov 4, 2020 at 3:51 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Wed, Nov 4, 2020 at 3:57 PM Jan Kiszka <jan.kiszka@siemens.com> 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 <brgl@bgdev.pl> wrote: > > >>> > > >>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > >>> > > >>> 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 <brgl@bgdev.pl> 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. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> [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
From: Bartosz Golaszewski <bgolaszewski@baylibre.com> 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(-)