gpio: exar: refactor the driver
cover gpio: exar: refactor the driver
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(-)
Signature · 1 line
2.29.1
PATCH 1/7 gpio: exar: add a newline after the copyright notice
From: Bartosz Golaszewski <[email protected]>
It's customary to have a newline between the copyright header and the includes. Add one to gpio-exar.
--- drivers/gpio/gpio-exar.c | 1 + 1 file changed, 1 insertion(+)
drivers/gpio/gpio-exar.c +1 -0
@@ -4,6 +4,7 @@* * Copyright (C) 2015 Sudip Mukherjee <[email protected]> */ +#include <linux/bitops.h> #include <linux/device.h> #include <linux/gpio/driver.h>
Signature · 1 line
2.29.1
PATCH 2/7 gpio: exar: include idr.h
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
This driver uses IDA APIs but doesn't include the relevant header. This fixes it.
--- drivers/gpio/gpio-exar.c | 1 + 1 file changed, 1 insertion(+)
drivers/gpio/gpio-exar.c +1 -0
@@ -8,6 +8,7 @@#include <linux/bitops.h> #include <linux/device.h> #include <linux/gpio/driver.h> +#include <linux/idr.h> #include <linux/init.h> #include <linux/kernel.h> #include <linux/module.h>
Signature · 1 line
2.29.1
PATCH 3/7 gpio: exar: switch to a simpler IDA interface
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
We don't need to specify any ranges when allocating IDs so we can switch to ida_alloc() and ida_free() instead of the ida_simple_ counterparts.
ida_simple_get(ida, 0, 0, gfp) is equivalent to ida_alloc_range(ida, 0, UINT_MAX, gfp) which is equivalent to ida_alloc(ida, gfp). Note: IDR will never actually allocate an ID larger than INT_MAX.
--- drivers/gpio/gpio-exar.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/gpio/gpio-exar.c +3 -3
@@ -149,7 +149,7 @@ static int gpio_exar_probe(struct platform_device *pdev)mutex_init (&exar_gpio->lock); - index =ida_simple_get (&ida_index,0 ,0 ,GFP_KERNEL ); + index =ida_alloc (&ida_index,GFP_KERNEL );if (index <0 ) { ret = index; goto err_mutex_destroy;
@@ -179,7 +179,7 @@ static int gpio_exar_probe(struct platform_device *pdev)return 0 ; err_destroy: -ida_simple_remove (&ida_index, index); +ida_free (&ida_index, index); err_mutex_destroy:mutex_destroy (&exar_gpio->lock);return ret;
@@ -189,7 +189,7 @@ static int gpio_exar_remove(struct platform_device *pdev) {struct exar_gpio_chip *exar_gpio =platform_get_drvdata (pdev); -ida_simple_remove (&ida_index, exar_gpio->index); +ida_free (&ida_index, exar_gpio->index);mutex_destroy (&exar_gpio->lock);return 0 ;
Signature · 1 line
2.29.1
PATCH 4/7 gpio: exar: use a helper variable for &pdev->dev
In reply to Bartosz GolaszewskiFrom: 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.
--- drivers/gpio/gpio-exar.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)
drivers/gpio/gpio-exar.c +7 -8
@@ -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;
Signature · 1 line
2.29.1
PATCH 5/7 gpio: exar: unduplicate address and offset computation
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
Provide and use helpers for calculating the register address and bit offset instead of hand coding it in every function.
--- drivers/gpio/gpio-exar.c | 40 ++++++++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 12 deletions(-)
drivers/gpio/gpio-exar.c +28 -12
@@ -33,6 +33,26 @@ struct exar_gpio_chip {unsigned int first_pin; }; +static unsigned int +exar_offset_to_sel_addr (struct exar_gpio_chip *exar_gpio,unsigned int offset) +{ +return (offset + exar_gpio->first_pin) /8 ?EXAR_OFFSET_MPIOSEL_HI + :EXAR_OFFSET_MPIOSEL_LO ; +} + +static unsigned int +exar_offset_to_lvl_addr (struct exar_gpio_chip *exar_gpio,unsigned int offset) +{ +return (offset + exar_gpio->first_pin) /8 ?EXAR_OFFSET_MPIOLVL_HI + :EXAR_OFFSET_MPIOLVL_LO ; +} + +static unsigned int +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) {
@@ -52,9 +72,8 @@ 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 = (offset + exar_gpio->first_pin) /8 ? -EXAR_OFFSET_MPIOSEL_HI :EXAR_OFFSET_MPIOSEL_LO ; -unsigned int bit = (offset + exar_gpio->first_pin) %8 ; +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 ;
@@ -75,9 +94,8 @@ static int exar_get(struct gpio_chip *chip, unsigned int reg)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 = (offset + exar_gpio->first_pin) /8 ? -EXAR_OFFSET_MPIOSEL_HI :EXAR_OFFSET_MPIOSEL_LO ; -unsigned int bit = (offset + exar_gpio->first_pin) %8 ; +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))return GPIO_LINE_DIRECTION_IN ;
@@ -88,9 +106,8 @@ static int exar_get_direction(struct gpio_chip *chip, unsigned int offset)static int exar_get_value (struct gpio_chip *chip,unsigned int offset) {struct exar_gpio_chip *exar_gpio =gpiochip_get_data (chip); -unsigned int addr = (offset + exar_gpio->first_pin) /8 ? -EXAR_OFFSET_MPIOLVL_HI :EXAR_OFFSET_MPIOLVL_LO ; -unsigned int bit = (offset + exar_gpio->first_pin) %8 ; +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)); }
@@ -99,9 +116,8 @@ static void exar_set_value(struct gpio_chip *chip, unsigned int offset, int value) {struct exar_gpio_chip *exar_gpio =gpiochip_get_data (chip); -unsigned int addr = (offset + exar_gpio->first_pin) /8 ? -EXAR_OFFSET_MPIOLVL_HI :EXAR_OFFSET_MPIOLVL_LO ; -unsigned int bit = (offset + exar_gpio->first_pin) %8 ; +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); }
Signature · 1 line
2.29.1
Re: [RFT PATCH 5/7] gpio: exar: unduplicate address and offset computation
Quoted 5 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Provide and use helpers for calculating the register address and bit offset instead of hand coding it in every function.
Can you check code generation on x86, for example?
Sometimes compilers are eager to use idiv assembly instruction which does simultaneously / and %.
I dunno if a) it's used for / 8 and % 8 since 8 is 2^3, b) splitting to functions makes the above optimisation impossible.
Quoted 87 lines
Signed-off-by Bartosz Golaszewski <[email protected]>--- drivers/gpio/gpio-exar.c | 40 ++++++++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 12 deletions(-)drivers/gpio/gpio-exar.c +28
-13@@ -33,6 +33,26 @@ struct exar_gpio_chip {unsigned int first_pin; }; +static unsigned int +exar_offset_to_sel_addr (struct exar_gpio_chip *exar_gpio,unsigned int offset) +{ +return (offset + exar_gpio->first_pin) /8 ?EXAR_OFFSET_MPIOSEL_HI + :EXAR_OFFSET_MPIOSEL_LO ; +} + +static unsigned int +exar_offset_to_lvl_addr (struct exar_gpio_chip *exar_gpio,unsigned int offset) +{ +return (offset + exar_gpio->first_pin) /8 ?EXAR_OFFSET_MPIOLVL_HI + :EXAR_OFFSET_MPIOLVL_LO ; +} + +static unsigned int +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) {@@ -52,9 +72,8 @@ 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 = (offset + exar_gpio->first_pin) /8 ? -EXAR_OFFSET_MPIOSEL_HI :EXAR_OFFSET_MPIOSEL_LO ; -unsigned int bit = (offset + exar_gpio->first_pin) %8 ; +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 ;@@ -75,9 +94,8 @@ static int exar_get(struct gpio_chip *chip, unsigned int reg)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 = (offset + exar_gpio->first_pin) /8 ? -EXAR_OFFSET_MPIOSEL_HI :EXAR_OFFSET_MPIOSEL_LO ; -unsigned int bit = (offset + exar_gpio->first_pin) %8 ; +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))return GPIO_LINE_DIRECTION_IN ;@@ -88,9 +106,8 @@ static int exar_get_direction(struct gpio_chip *chip, unsigned int offset)static int exar_get_value (struct gpio_chip *chip,unsigned int offset) {struct exar_gpio_chip *exar_gpio =gpiochip_get_data (chip); -unsigned int addr = (offset + exar_gpio->first_pin) /8 ? -EXAR_OFFSET_MPIOLVL_HI :EXAR_OFFSET_MPIOLVL_LO ; -unsigned int bit = (offset + exar_gpio->first_pin) %8 ; +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)); }@@ -99,9 +116,8 @@ static void exar_set_value(struct gpio_chip *chip, unsigned int offset, int value) {struct exar_gpio_chip *exar_gpio =gpiochip_get_data (chip); -unsigned int addr = (offset + exar_gpio->first_pin) /8 ? -EXAR_OFFSET_MPIOLVL_HI :EXAR_OFFSET_MPIOLVL_LO ; -unsigned int bit = (offset + exar_gpio->first_pin) %8 ; +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); } --2.29.1
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [RFT PATCH 5/7] gpio: exar: unduplicate address and offset computation
Quoted 8 lines · Andy Shevchenko
Quoted 5 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Provide and use helpers for calculating the register address and bit offset instead of hand coding it in every function.
Quoted 7 lines
Can you check code generation on x86, for example?
Sometimes compilers are eager to use idiv assembly instruction which does simultaneously / and %.
I dunno if a) it's used for / 8 and % 8 since 8 is 2^3, b) splitting to functions makes the above optimisation impossible.
Is this optimization really needed though? It's not like it's a hot path if it's protected by a mutex anyway. I prefer cleaner code here.
Bartosz
Re: [RFT PATCH 5/7] gpio: exar: unduplicate address and offset computation
In reply to Bartosz GolaszewskiOn Mon, Oct 26, 2020 at 4:23 PM Bartosz Golaszewski <[email protected]> wrote:
...
Quoted 9 lines
drivers/gpio/gpio-exar.c +9
-0+static unsigned int +exar_offset_to_sel_addr (struct exar_gpio_chip *exar_gpio,unsigned int offset) +{ +return (offset + exar_gpio->first_pin) /8 ?EXAR_OFFSET_MPIOSEL_HI + :EXAR_OFFSET_MPIOSEL_LO ; +} + +static unsigned int +exar_offset_to_lvl_addr (struct exar_gpio_chip *exar_gpio,unsigned int offset)
drivers/gpio/gpio-exar.c struct exar_gpio_chip { · line 54
Quoted 10 lines
drivers/gpio/gpio-exar.c +10
-0+{ +return (offset + exar_gpio->first_pin) /8 ?EXAR_OFFSET_MPIOLVL_HI + :EXAR_OFFSET_MPIOLVL_LO ; +} + +static unsigned int +exar_offset_to_bit (struct exar_gpio_chip *exar_gpio,unsigned int offset) +{ +return (offset + exar_gpio->first_pin) %8 ; +}
Answering to your question...
It can be done line this:
static unsigned int exar_offset_to_bank_and_bit(..., *bit)
{
*bit = (offset + exar_gpio->first_pin) % 8;
return (offset + exar_gpio->first_pin) / 8;
}static unsigned int exar_offset_to_lvl_addr_and_bit(, *bit)
{
return exar_offset_to_bank_and_bit(..., bit) ?
EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO;
}...
drivers/gpio/gpio-exar.c static int exar_get_direction(struct gpio_chip *chip, unsigned int offset) · line 110
Quoted 2 lines
drivers/gpio/gpio-exar.c +2
-0+unsigned int addr =exar_offset_to_lvl_addr (exar_gpio, offset); +unsigned int bit =exar_offset_to_bit (exar_gpio, offset);
unsigned int addr, bit;
addr = exar_offset_to_lvl_addr_and_bit(..., &bit);
Signature · 2 lines
With Best Regards, Andy Shevchenko
RE: [RFT PATCH 5/7] gpio: exar: unduplicate address and offset computation
From: Andy Shevchenko
Quoted 28 lines
Sent: 02 November 2020 10:59
On Mon, Oct 26, 2020 at 4:23 PM Bartosz Golaszewski <[email protected]> wrote:
...
Quoted 9 lines
drivers/gpio/gpio-exar.c +9
-0+static unsigned int +exar_offset_to_sel_addr (struct exar_gpio_chip *exar_gpio,unsigned int offset) +{ +return (offset + exar_gpio->first_pin) /8 ?EXAR_OFFSET_MPIOSEL_HI + :EXAR_OFFSET_MPIOSEL_LO ; +} + +static unsigned int +exar_offset_to_lvl_addr (struct exar_gpio_chip *exar_gpio,unsigned int offset)drivers/gpio/gpio-exar.c struct exar_gpio_chip { · line 54
Quoted 10 lines
drivers/gpio/gpio-exar.c +10
-0+{ +return (offset + exar_gpio->first_pin) /8 ?EXAR_OFFSET_MPIOLVL_HI + :EXAR_OFFSET_MPIOLVL_LO ; +} + +static unsigned int +exar_offset_to_bit (struct exar_gpio_chip *exar_gpio,unsigned int offset) +{ +return (offset + exar_gpio->first_pin) %8 ; +}Answering to your question...
Quoted 7 lines
It can be done line this:
static unsigned int
exar_offset_to_bank_and_bit(..., *bit){ *bit = (offset + exar_gpio->first_pin) % 8; return (offset + exar_gpio->first_pin) / 8; }
That is likely to require the compiler reload exar_gpio->first_pin after the write to *bit.
Quoted 5 lines
static unsigned int
exar_offset_to_lvl_addr_and_bit(, *bit){ return exar_offset_to_bank_and_bit(..., bit) ? EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO; }
Gah why is it using divide then ?: ? AFAICT (from the above) there are at most 16 pins.
Much better would be using:
tmp = offset + exar_gpio->first_pin; *bit = tmp & 7; return tmp & 8;
Inlined the compiler may well compute:
exar_offset_to_bank_and_bit() ? HI : LO; as: LO + (HI - LO) * exar_offset_to_bank_and_bit().
The latter term is likely to be just (tmp & 8) >> n.
I also bet the code actually wants (1 << bit).
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
PATCH 6/7 gpio: exar: switch to using regmap
In reply to Bartosz GolaszewskiFrom: 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.
--- drivers/gpio/Kconfig | 1 + drivers/gpio/gpio-exar.c | 91 ++++++++++++++++------------------------ 2 files changed, 38 insertions(+), 54 deletions(-)
drivers/gpio/Kconfig +1 -0
@@ -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.
drivers/gpio/gpio-exar.c +37 -54
159 lines not on this page · raw message
Signature · 1 line
2.29.1
Re: [RFT PATCH 6/7] gpio: exar: switch to using regmap
Quoted 6 lines · Bartosz Golaszewski
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.
...
drivers/gpio/gpio-exar.c static void exar_set_value(struct gpio_chip *chip, unsigned int offset, · line 87
Quoted 4 lines
drivers/gpio/gpio-exar.c +4
-0+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() ?
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [RFT PATCH 6/7] gpio: exar: switch to using regmap
In reply to Bartosz GolaszewskiQuoted 8 lines · Bartosz Golaszewski
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]
Quoted 5 lines
static int exar_direction_output(struct
gpio_chip*chip, unsigned int offset,
int value)
{
drivers/gpio/gpio-exar.c static void exar_set_value(struct gpio_chip *chip, unsigned int offset, · line 100
Quoted 10 lines
drivers/gpio/gpio-exar.c +7
-2-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
PATCH 7/7 gpio: exar: use devm action for freeing the IDA and drop remove()
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
We can simplify the error path in probe() and drop remove() entirely if we provide a devm action for freeing the device ID.
--- drivers/gpio/gpio-exar.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-)
drivers/gpio/gpio-exar.c +12 -15
@@ -110,6 +110,13 @@ static int exar_direction_input(struct gpio_chip *chip, unsigned int offset)return 0 ; } +static void exar_devm_ida_free (void *data) +{ +struct exar_gpio_chip *exar_gpio = data; + +ida_free (&ida_index, exar_gpio->index); +} +static const struct regmap_config exar_regmap_config = { .name ="exar-gpio" , .reg_bits =8 ,
@@ -157,6 +164,10 @@ static int gpio_exar_probe(struct platform_device *pdev)if (index <0 )return index; + ret =devm_add_action_or_reset (dev, exar_devm_ida_free, exar_gpio); +if (ret) +return ret; +sprintf (exar_gpio->name,"exar_gpio%d" , index); exar_gpio->gpio_chip.label = exar_gpio->name; exar_gpio->gpio_chip.parent = dev;
@@ -173,29 +184,15 @@ static int gpio_exar_probe(struct platform_device *pdev) ret =devm_gpiochip_add_data (dev, &exar_gpio->gpio_chip, exar_gpio);if (ret) - goto err_destroy; +return ret;platform_set_drvdata (pdev, exar_gpio); -return 0 ; - -err_destroy: -ida_free (&ida_index, index); -return ret; -} - -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); -return 0 ; }static struct platform_driver gpio_exar_driver = { .probe = gpio_exar_probe, - .remove = gpio_exar_remove, .driver = { .name =DRIVER_NAME , },
Signature · 1 line
2.29.1
Re: [PATCH 7/7] gpio: exar: use devm action for freeing the IDA and drop remove()
Quoted 5 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
We can simplify the error path in
probe()and dropremove()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)
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH 7/7] gpio: exar: use devm action for freeing the IDA and drop remove()
Quoted 5 lines · Andy Shevchenko
Quoted 3 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Quoted 8 lines
Quoted 2 lines
We can simplify the error path in
probe()and dropremove()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
Quoted 7 lines · Bartosz Golaszewski
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.
Quoted 16 lines
Bartosz Golaszewski (7):
gpio: exar: add a newline after the copyright notice
gpio: exar: includeidr.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 dropremove()drivers/gpio/Kconfig | 1 + drivers/gpio/gpio-exar.c | 155 +++++++++++++++++++-------------------- 2 files changed, 77 insertions(+), 79 deletions(-)--
2.29.1
Signature · 2 lines
With Best Regards, Andy Shevchenko
Andy Shevchenko
Reviewed-byQuoted 11 lines · Andy Shevchenko
Quoted 7 lines · Bartosz Golaszewski
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.
From code perspective looks good to me, FWIW,
(One nit to one patch, but I think it should be fine)
Quoted 8 lines
Quoted 8 lines
Bartosz Golaszewski (7):
gpio: exar: add a newline after the copyright notice
gpio: exar: includeidr.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 dropremove()
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 11 lines · Andy Shevchenko
Quoted 7 lines · Bartosz Golaszewski
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
Quoted 17 lines
Quoted 16 lines
Bartosz Golaszewski (7):
gpio: exar: add a newline after the copyright notice
gpio: exar: includeidr.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 dropremove()drivers/gpio/Kconfig | 1 + drivers/gpio/gpio-exar.c | 155 +++++++++++++++++++-------------------- 2 files changed, 77 insertions(+), 79 deletions(-)--
2.29.1
Signature · 2 lines
Siemens AG, T RDA IOT Corporate Competence Center Embedded Linux
Quoted 10 lines · Jan Kiszka
Quoted 9 lines · Andy Shevchenko
Quoted 7 lines · Bartosz Golaszewski
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.
Quoted 6 lines
Quoted 2 lines
+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
Signature · 2 lines
Siemens AG, T RDA IOT Corporate Competence Center Embedded Linux
Quoted 14 lines · Jan Kiszka
Quoted 13 lines · Jan Kiszka
Quoted 11 lines · Andy Shevchenko
Quoted 7 lines · Bartosz Golaszewski
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.
Quoted 7 lines
Quoted 3 lines
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.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 20 lines · Andy Shevchenko
Quoted 18 lines · Jan Kiszka
Quoted 16 lines · Jan Kiszka
Quoted 11 lines · Andy Shevchenko
Quoted 7 lines · Bartosz Golaszewski
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.
Quoted 7 lines
Quoted 3 lines
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