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.
v1 -> v2:
- add new regmap helper: regmap_assign_bits()
- fix lvl vs sel register access
- set value in direction_output callback
v2 -> v3:
- drop the regmap helper from series
v3 -> v4:
- renamed the regmap variable to 'regmap' as leaving the old name caused me
to miss an assignment leading to a crash (culprit spotted by Andy Shevchenko)
v4 -> v5:
- set the reg_bits to 11 in regmap config
- collect review tags from Andy
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 | 160 ++++++++++++++++++++-------------------
2 files changed, 82 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]>
Reviewed-by: Andy Shevchenko <[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]>
We can simplify the error path in probe() and drop remove() entirely if
we provide a devm action for freeing the device ID.
Signed-off-by: Bartosz Golaszewski <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
drivers/gpio/gpio-exar.c | 27 ++++++++++++---------------
1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index 79fb0964ace3..e4c1ac88d2ba 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -111,6 +111,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",
/*
@@ -163,6 +170,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;
@@ -178,29 +189,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,
},
--
2.29.1
On 23.11.20 12:38, Bartosz Golaszewski wrote:
> On Mon, Nov 16, 2020 at 11:42 AM 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.
>>
>
> Hi Jan!
>
> Could you give this last version a final spin before I merge it?
>
[ 14.250117] exar_serial 0000:02:00.0: enabling device (0000 -> 0002)
[ 14.336622] 0000:02:00.0: ttyS2 at MMIO 0x90000000 (irq = 44, base_baud = 7812500) is a XR17V35X
[ 14.391588] 0000:02:00.0: ttyS3 at MMIO 0x90000400 (irq = 44, base_baud = 7812500) is a XR17V35X
[ 19.250510] gpio_exar: probe of gpio_exar.1.auto failed with error -22
That's "new"...
Jan
--
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux
On 23.11.20 12:58, Jan Kiszka wrote:
> On 23.11.20 12:38, Bartosz Golaszewski wrote:
>> On Mon, Nov 16, 2020 at 11:42 AM 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.
>>>
>>
>> Hi Jan!
>>
>> Could you give this last version a final spin before I merge it?
>>
>
> [ 14.250117] exar_serial 0000:02:00.0: enabling device (0000 -> 0002)
> [ 14.336622] 0000:02:00.0: ttyS2 at MMIO 0x90000000 (irq = 44, base_baud = 7812500) is a XR17V35X
> [ 14.391588] 0000:02:00.0: ttyS3 at MMIO 0x90000400 (irq = 44, base_baud = 7812500) is a XR17V35X
> [ 19.250510] gpio_exar: probe of gpio_exar.1.auto failed with error -22
>
> That's "new"...
>
Bisected to "gpio: exar: switch to using regmap" again.
Jan
--
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux
Thanks!On Mon, Nov 23, 2020 at 1:03 PM Jan Kiszka
<[email protected]> wrote:
>
> On 23.11.20 12:38, Bartosz Golaszewski wrote:
> > On Mon, Nov 16, 2020 at 11:42 AM 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.
> >>
> >
> > Hi Jan!
> >
> > Could you give this last version a final spin before I merge it?
> >
>
> [ 14.250117] exar_serial 0000:02:00.0: enabling device (0000 -> 0002)
> [ 14.336622] 0000:02:00.0: ttyS2 at MMIO 0x90000000 (irq = 44, base_baud = 7812500) is a XR17V35X
> [ 14.391588] 0000:02:00.0: ttyS3 at MMIO 0x90000400 (irq = 44, base_baud = 7812500) is a XR17V35X
> [ 19.250510] gpio_exar: probe of gpio_exar.1.auto failed with error -22
>
> That's "new"...
>
And if you change reg_bits from 11 to 16?
Bartosz
On 23.11.20 13:12, Bartosz Golaszewski wrote:
> Thanks!On Mon, Nov 23, 2020 at 1:03 PM Jan Kiszka
> <[email protected]> wrote:
>>
>> On 23.11.20 12:38, Bartosz Golaszewski wrote:
>>> On Mon, Nov 16, 2020 at 11:42 AM 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.
>>>>
>>>
>>> Hi Jan!
>>>
>>> Could you give this last version a final spin before I merge it?
>>>
>>
>> [ 14.250117] exar_serial 0000:02:00.0: enabling device (0000 -> 0002)
>> [ 14.336622] 0000:02:00.0: ttyS2 at MMIO 0x90000000 (irq = 44, base_baud = 7812500) is a XR17V35X
>> [ 14.391588] 0000:02:00.0: ttyS3 at MMIO 0x90000400 (irq = 44, base_baud = 7812500) is a XR17V35X
>> [ 19.250510] gpio_exar: probe of gpio_exar.1.auto failed with error -22
>>
>> That's "new"...
>>
>
> And if you change reg_bits from 11 to 16?
>
16 works. Didn't we have that already?
Jan
--
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux
On Mon, Nov 23, 2020 at 1:12 PM Jan Kiszka <[email protected]> wrote:
>
> On 23.11.20 12:58, Jan Kiszka wrote:
> > On 23.11.20 12:38, Bartosz Golaszewski wrote:
> >> On Mon, Nov 16, 2020 at 11:42 AM 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.
> >>>
> >>
> >> Hi Jan!
> >>
> >> Could you give this last version a final spin before I merge it?
> >>
> >
> > [ 14.250117] exar_serial 0000:02:00.0: enabling device (0000 -> 0002)
> > [ 14.336622] 0000:02:00.0: ttyS2 at MMIO 0x90000000 (irq = 44, base_baud = 7812500) is a XR17V35X
> > [ 14.391588] 0000:02:00.0: ttyS3 at MMIO 0x90000400 (irq = 44, base_baud = 7812500) is a XR17V35X
> > [ 19.250510] gpio_exar: probe of gpio_exar.1.auto failed with error -22
> >
> > That's "new"...
> >
>
> Bisected to "gpio: exar: switch to using regmap" again.
>
I'm not sure if you saw my email which I sent at the same time as you
- but does reverting reg_bits to 16 help?
Bart
On Mon, Nov 23, 2020 at 01:57:07PM +0100, Jan Kiszka wrote:
> On 23.11.20 13:12, Bartosz Golaszewski wrote:
> > Thanks!On Mon, Nov 23, 2020 at 1:03 PM Jan Kiszka
> > <[email protected]> wrote:
> >>
> >> On 23.11.20 12:38, Bartosz Golaszewski wrote:
> >>> On Mon, Nov 16, 2020 at 11:42 AM 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.
> >>>>
> >>>
> >>> Hi Jan!
> >>>
> >>> Could you give this last version a final spin before I merge it?
> >>>
> >>
> >> [ 14.250117] exar_serial 0000:02:00.0: enabling device (0000 -> 0002)
> >> [ 14.336622] 0000:02:00.0: ttyS2 at MMIO 0x90000000 (irq = 44, base_baud = 7812500) is a XR17V35X
> >> [ 14.391588] 0000:02:00.0: ttyS3 at MMIO 0x90000400 (irq = 44, base_baud = 7812500) is a XR17V35X
> >> [ 19.250510] gpio_exar: probe of gpio_exar.1.auto failed with error -22
> >>
> >> That's "new"...
> >>
> >
> > And if you change reg_bits from 11 to 16?
> >
>
> 16 works. Didn't we have that already?
Yes, but it puzzles me what the idea behind it and don't we see the regmap
issue here. In any case, I'm fine with 16 there, although it's not clear
to me why 11 is not working...
--
With Best Regards,
Andy Shevchenko
On Mon, Nov 16, 2020 at 11:42 AM 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.
>
Hi Jan!
Could you give this last version a final spin before I merge it?
Bartosz
On Mon, Nov 23, 2020 at 2:00 PM Jan Kiszka <[email protected]> wrote:
>
> On 23.11.20 13:12, Bartosz Golaszewski wrote:
> > Thanks!On Mon, Nov 23, 2020 at 1:03 PM Jan Kiszka
> > <[email protected]> wrote:
> >>
> >> On 23.11.20 12:38, Bartosz Golaszewski wrote:
> >>> On Mon, Nov 16, 2020 at 11:42 AM 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.
> >>>>
> >>>
> >>> Hi Jan!
> >>>
> >>> Could you give this last version a final spin before I merge it?
> >>>
> >>
> >> [ 14.250117] exar_serial 0000:02:00.0: enabling device (0000 -> 0002)
> >> [ 14.336622] 0000:02:00.0: ttyS2 at MMIO 0x90000000 (irq = 44, base_baud = 7812500) is a XR17V35X
> >> [ 14.391588] 0000:02:00.0: ttyS3 at MMIO 0x90000400 (irq = 44, base_baud = 7812500) is a XR17V35X
> >> [ 19.250510] gpio_exar: probe of gpio_exar.1.auto failed with error -22
> >>
> >> That's "new"...
> >>
> >
> > And if you change reg_bits from 11 to 16?
> >
>
> 16 works. Didn't we have that already?
>
> Jan
Yes we have, Andy suggested 11 is fine because it fits the highest
address we need to access but it seems regmap doesn't like this value.
Ok so I'll change it when applying, is that fine with you?
Bart