* [PATCH v2 0/4] gpio-langwell: bugfix and amendments
@ 2013-05-22 7:47 Andy Shevchenko
2013-05-22 7:47 ` [PATCH v2 1/4] gpio-langwell: initialize lock before usage Andy Shevchenko
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Andy Shevchenko @ 2013-05-22 7:47 UTC (permalink / raw)
To: Linus Walleij, David Cohen, Mika Westerberg, linux-kernel; +Cc: Andy Shevchenko
There is locking bug fix and few amendments. Each commit message verbose
enoough I think.
Since v1:
- patch 2/4 ("amend error messages") moved to 4/4
- minor fix in patch 3/4 ("use managed ...")
- fix patch 4/4 ("amend error messages") to be real amendment
Andy Shevchenko (4):
gpio-langwell: initialize lock before usage
gpio-langwell: do not use direct access to iomapped memory
gpio-langwell: use managed functions pcim_* and devm_*
gpio-langwell: amend error messages
drivers/gpio/gpio-langwell.c | 102 +++++++++++++------------------------------
1 file changed, 31 insertions(+), 71 deletions(-)
--
1.8.2.rc0.22.gb3600c3
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/4] gpio-langwell: initialize lock before usage
2013-05-22 7:47 [PATCH v2 0/4] gpio-langwell: bugfix and amendments Andy Shevchenko
@ 2013-05-22 7:47 ` Andy Shevchenko
2013-05-22 7:58 ` Mika Westerberg
2013-05-22 7:47 ` [PATCH v2 2/4] gpio-langwell: do not use direct access to iomapped memory Andy Shevchenko
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2013-05-22 7:47 UTC (permalink / raw)
To: Linus Walleij, David Cohen, Mika Westerberg, linux-kernel; +Cc: Andy Shevchenko
Otherwise we will end up with traceback from LOCKDEP:
INFO: trying to register non-static key.
the code is fine but needs lockdep annotation.
turning off the locking correctness validator.
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.10.0-rc2-next-20130521-00028-g09aa9fc #487
00000000 00000000 f6c55c54 c1541fe4 f6040bf8 f6c55c8c c1069ef1 c1726bc1
c1726cc8 c1726c9e 00000000 f6c584e0 f6c58000 f6c55ce8 00000000 f6040bf8
f6040bf8 00000046 f6c58000 f6c55d00 c106a18d 00000a2b 00000003 00004f02
Call Trace:
[<c1541fe4>] dump_stack+0x49/0x77
[<c1069ef1>] register_lock_class+0x58/0x260
[<c106a18d>] __lock_acquire+0x94/0xcff
[<c106adc8>] ? __lock_acquire+0xccf/0xcff
[<c106b2ad>] lock_acquire+0xcc/0x10d
[<c1269c7c>] ? lnw_irq_type+0x63/0xe9
[<c1545da0>] _raw_spin_lock_irqsave+0x32/0x42
[<c1269c7c>] ? lnw_irq_type+0x63/0xe9
[<c1269c7c>] lnw_irq_type+0x63/0xe9
[<c108f454>] __irq_set_trigger+0x98/0x123
[<c1090225>] irq_set_irq_type+0x2f/0x51
[<c1090225>] ? irq_set_irq_type+0x2f/0x51
[<c1269d02>] ? lnw_irq_type+0xe9/0xe9
[<c1269d34>] lnw_gpio_irq_map+0x32/0x3b
[<c10914f2>] irq_domain_add_legacy+0xe2/0x107
[<c1091b53>] irq_domain_add_simple+0x47/0x60
[<c1269f6e>] lnw_gpio_probe+0x119/0x217
[<c1271018>] pci_device_probe+0x5a/0x92
...
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/gpio/gpio-langwell.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpio-langwell.c b/drivers/gpio/gpio-langwell.c
index 62ef10a..c1f33dd 100644
--- a/drivers/gpio/gpio-langwell.c
+++ b/drivers/gpio/gpio-langwell.c
@@ -380,6 +380,8 @@ static int lnw_gpio_probe(struct pci_dev *pdev,
lnw->chip.can_sleep = 0;
lnw->pdev = pdev;
+ spin_lock_init(&lnw->lock);
+
lnw->domain = irq_domain_add_simple(pdev->dev.of_node, ngpio, irq_base,
&lnw_gpio_irq_ops, lnw);
if (!lnw->domain) {
@@ -399,8 +401,6 @@ static int lnw_gpio_probe(struct pci_dev *pdev,
irq_set_handler_data(pdev->irq, lnw);
irq_set_chained_handler(pdev->irq, lnw_irq_handler);
- spin_lock_init(&lnw->lock);
-
pm_runtime_put_noidle(&pdev->dev);
pm_runtime_allow(&pdev->dev);
--
1.8.2.rc0.22.gb3600c3
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/4] gpio-langwell: do not use direct access to iomapped memory
2013-05-22 7:47 [PATCH v2 0/4] gpio-langwell: bugfix and amendments Andy Shevchenko
2013-05-22 7:47 ` [PATCH v2 1/4] gpio-langwell: initialize lock before usage Andy Shevchenko
@ 2013-05-22 7:47 ` Andy Shevchenko
2013-05-22 7:58 ` Mika Westerberg
2013-05-22 7:47 ` [PATCH v2 3/4] gpio-langwell: use managed functions pcim_* and devm_* Andy Shevchenko
2013-05-22 7:47 ` [PATCH v2 4/4] gpio-langwell: amend error messages Andy Shevchenko
3 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2013-05-22 7:47 UTC (permalink / raw)
To: Linus Walleij, David Cohen, Mika Westerberg, linux-kernel; +Cc: Andy Shevchenko
We better to use readl() function instead of bad looking direct access.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/gpio/gpio-langwell.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/gpio/gpio-langwell.c b/drivers/gpio/gpio-langwell.c
index c1f33dd..8203084 100644
--- a/drivers/gpio/gpio-langwell.c
+++ b/drivers/gpio/gpio-langwell.c
@@ -65,7 +65,7 @@ enum GPIO_REG {
struct lnw_gpio {
struct gpio_chip chip;
- void *reg_base;
+ void __iomem *reg_base;
spinlock_t lock;
struct pci_dev *pdev;
struct irq_domain *domain;
@@ -318,9 +318,9 @@ static const struct dev_pm_ops lnw_gpio_pm_ops = {
};
static int lnw_gpio_probe(struct pci_dev *pdev,
- const struct pci_device_id *id)
+ const struct pci_device_id *id)
{
- void *base;
+ void __iomem *base;
resource_size_t start, len;
struct lnw_gpio *lnw;
u32 gpio_base;
@@ -346,8 +346,10 @@ static int lnw_gpio_probe(struct pci_dev *pdev,
retval = -EFAULT;
goto err_ioremap;
}
- irq_base = *(u32 *)base;
- gpio_base = *((u32 *)base + 1);
+
+ irq_base = readl(base);
+ gpio_base = readl(sizeof(u32) + base);
+
/* release the IO mapping, since we already get the info from bar1 */
iounmap(base);
/* get the register base from bar0 */
--
1.8.2.rc0.22.gb3600c3
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 3/4] gpio-langwell: use managed functions pcim_* and devm_*
2013-05-22 7:47 [PATCH v2 0/4] gpio-langwell: bugfix and amendments Andy Shevchenko
2013-05-22 7:47 ` [PATCH v2 1/4] gpio-langwell: initialize lock before usage Andy Shevchenko
2013-05-22 7:47 ` [PATCH v2 2/4] gpio-langwell: do not use direct access to iomapped memory Andy Shevchenko
@ 2013-05-22 7:47 ` Andy Shevchenko
2013-05-22 8:05 ` Mika Westerberg
2013-05-22 7:47 ` [PATCH v2 4/4] gpio-langwell: amend error messages Andy Shevchenko
3 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2013-05-22 7:47 UTC (permalink / raw)
To: Linus Walleij, David Cohen, Mika Westerberg, linux-kernel; +Cc: Andy Shevchenko
This makes the error handling much more simpler than open-coding everything and
in addition makes the probe function smaller an tidier.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/gpio/gpio-langwell.c | 82 ++++++++++++--------------------------------
1 file changed, 21 insertions(+), 61 deletions(-)
diff --git a/drivers/gpio/gpio-langwell.c b/drivers/gpio/gpio-langwell.c
index 8203084..8672282 100644
--- a/drivers/gpio/gpio-langwell.c
+++ b/drivers/gpio/gpio-langwell.c
@@ -320,56 +320,35 @@ static const struct dev_pm_ops lnw_gpio_pm_ops = {
static int lnw_gpio_probe(struct pci_dev *pdev,
const struct pci_device_id *id)
{
- void __iomem *base;
- resource_size_t start, len;
struct lnw_gpio *lnw;
u32 gpio_base;
u32 irq_base;
int retval;
int ngpio = id->driver_data;
- retval = pci_enable_device(pdev);
+ retval = pcim_enable_device(pdev);
if (retval)
return retval;
- retval = pci_request_regions(pdev, "langwell_gpio");
+ retval = pcim_iomap_regions(pdev, 1 << 0 | 1 << 1, pci_name(pdev));
if (retval) {
- dev_err(&pdev->dev, "error requesting resources\n");
- goto err_pci_req_region;
- }
- /* get the gpio_base from bar1 */
- start = pci_resource_start(pdev, 1);
- len = pci_resource_len(pdev, 1);
- base = ioremap_nocache(start, len);
- if (!base) {
- dev_err(&pdev->dev, "error mapping bar1\n");
- retval = -EFAULT;
- goto err_ioremap;
+ dev_err(&pdev->dev, "I/O memory mapping error\n");
+ return retval;
}
- irq_base = readl(base);
- gpio_base = readl(sizeof(u32) + base);
+ irq_base = readl(pcim_iomap_table(pdev)[1]);
+ gpio_base = readl(sizeof(u32) + pcim_iomap_table(pdev)[1]);
/* release the IO mapping, since we already get the info from bar1 */
- iounmap(base);
- /* get the register base from bar0 */
- start = pci_resource_start(pdev, 0);
- len = pci_resource_len(pdev, 0);
- base = devm_ioremap_nocache(&pdev->dev, start, len);
- if (!base) {
- dev_err(&pdev->dev, "error mapping bar0\n");
- retval = -EFAULT;
- goto err_ioremap;
- }
+ pcim_iounmap_regions(pdev, 1 << 1);
lnw = devm_kzalloc(&pdev->dev, sizeof(*lnw), GFP_KERNEL);
if (!lnw) {
dev_err(&pdev->dev, "can't allocate langwell_gpio chip data\n");
- retval = -ENOMEM;
- goto err_ioremap;
+ return -ENOMEM;
}
- lnw->reg_base = base;
+ lnw->reg_base = pcim_iomap_table(pdev)[0];
lnw->chip.label = dev_name(&pdev->dev);
lnw->chip.request = lnw_gpio_request;
lnw->chip.direction_input = lnw_gpio_direction_input;
@@ -386,16 +365,14 @@ static int lnw_gpio_probe(struct pci_dev *pdev,
lnw->domain = irq_domain_add_simple(pdev->dev.of_node, ngpio, irq_base,
&lnw_gpio_irq_ops, lnw);
- if (!lnw->domain) {
- retval = -ENOMEM;
- goto err_ioremap;
- }
+ if (!lnw->domain)
+ return -ENOMEM;
pci_set_drvdata(pdev, lnw);
retval = gpiochip_add(&lnw->chip);
if (retval) {
dev_err(&pdev->dev, "langwell gpiochip_add error %d\n", retval);
- goto err_ioremap;
+ return retval;
}
lnw_irq_init_hw(lnw);
@@ -407,12 +384,6 @@ static int lnw_gpio_probe(struct pci_dev *pdev,
pm_runtime_allow(&pdev->dev);
return 0;
-
-err_ioremap:
- pci_release_regions(pdev);
-err_pci_req_region:
- pci_disable_device(pdev);
- return retval;
}
static struct pci_driver lnw_gpio_driver = {
@@ -430,23 +401,20 @@ static int wp_gpio_probe(struct platform_device *pdev)
struct lnw_gpio *lnw;
struct gpio_chip *gc;
struct resource *rc;
- int retval = 0;
-
- rc = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!rc)
- return -EINVAL;
+ int retval;
- lnw = kzalloc(sizeof(struct lnw_gpio), GFP_KERNEL);
+ lnw = devm_kzalloc(&pdev->dev, sizeof(struct lnw_gpio), GFP_KERNEL);
if (!lnw) {
dev_err(&pdev->dev,
"can't allocate whitneypoint_gpio chip data\n");
return -ENOMEM;
}
- lnw->reg_base = ioremap_nocache(rc->start, resource_size(rc));
- if (lnw->reg_base == NULL) {
- retval = -EINVAL;
- goto err_kmalloc;
- }
+
+ rc = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ lnw->reg_base = devm_ioremap_resource(&pdev->dev, rc);
+ if (IS_ERR(lnw->reg_base))
+ return PTR_ERR(lnw->reg_base);
+
spin_lock_init(&lnw->lock);
gc = &lnw->chip;
gc->label = dev_name(&pdev->dev);
@@ -463,15 +431,10 @@ static int wp_gpio_probe(struct platform_device *pdev)
if (retval) {
dev_err(&pdev->dev, "whitneypoint gpiochip_add error %d\n",
retval);
- goto err_ioremap;
+ return retval;
}
platform_set_drvdata(pdev, lnw);
return 0;
-err_ioremap:
- iounmap(lnw->reg_base);
-err_kmalloc:
- kfree(lnw);
- return retval;
}
static int wp_gpio_remove(struct platform_device *pdev)
@@ -481,9 +444,6 @@ static int wp_gpio_remove(struct platform_device *pdev)
err = gpiochip_remove(&lnw->chip);
if (err)
dev_err(&pdev->dev, "failed to remove gpio_chip.\n");
- iounmap(lnw->reg_base);
- kfree(lnw);
- platform_set_drvdata(pdev, NULL);
return 0;
}
--
1.8.2.rc0.22.gb3600c3
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 4/4] gpio-langwell: amend error messages
2013-05-22 7:47 [PATCH v2 0/4] gpio-langwell: bugfix and amendments Andy Shevchenko
` (2 preceding siblings ...)
2013-05-22 7:47 ` [PATCH v2 3/4] gpio-langwell: use managed functions pcim_* and devm_* Andy Shevchenko
@ 2013-05-22 7:47 ` Andy Shevchenko
2013-05-22 8:06 ` Mika Westerberg
3 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2013-05-22 7:47 UTC (permalink / raw)
To: Linus Walleij, David Cohen, Mika Westerberg, linux-kernel; +Cc: Andy Shevchenko
There is no need to use hardcoded device name in the error messages, because
dev_err() prefixes the message with the device name anyway.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/gpio/gpio-langwell.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/gpio/gpio-langwell.c b/drivers/gpio/gpio-langwell.c
index 8672282..aa34aa4 100644
--- a/drivers/gpio/gpio-langwell.c
+++ b/drivers/gpio/gpio-langwell.c
@@ -344,7 +344,7 @@ static int lnw_gpio_probe(struct pci_dev *pdev,
lnw = devm_kzalloc(&pdev->dev, sizeof(*lnw), GFP_KERNEL);
if (!lnw) {
- dev_err(&pdev->dev, "can't allocate langwell_gpio chip data\n");
+ dev_err(&pdev->dev, "can't allocate chip data\n");
return -ENOMEM;
}
@@ -371,7 +371,7 @@ static int lnw_gpio_probe(struct pci_dev *pdev,
pci_set_drvdata(pdev, lnw);
retval = gpiochip_add(&lnw->chip);
if (retval) {
- dev_err(&pdev->dev, "langwell gpiochip_add error %d\n", retval);
+ dev_err(&pdev->dev, "gpiochip_add error %d\n", retval);
return retval;
}
@@ -405,8 +405,7 @@ static int wp_gpio_probe(struct platform_device *pdev)
lnw = devm_kzalloc(&pdev->dev, sizeof(struct lnw_gpio), GFP_KERNEL);
if (!lnw) {
- dev_err(&pdev->dev,
- "can't allocate whitneypoint_gpio chip data\n");
+ dev_err(&pdev->dev, "can't allocate chip data\n");
return -ENOMEM;
}
@@ -429,8 +428,7 @@ static int wp_gpio_probe(struct platform_device *pdev)
gc->can_sleep = 0;
retval = gpiochip_add(gc);
if (retval) {
- dev_err(&pdev->dev, "whitneypoint gpiochip_add error %d\n",
- retval);
+ dev_err(&pdev->dev, "gpiochip_add error %d\n", retval);
return retval;
}
platform_set_drvdata(pdev, lnw);
--
1.8.2.rc0.22.gb3600c3
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/4] gpio-langwell: initialize lock before usage
2013-05-22 7:47 ` [PATCH v2 1/4] gpio-langwell: initialize lock before usage Andy Shevchenko
@ 2013-05-22 7:58 ` Mika Westerberg
0 siblings, 0 replies; 11+ messages in thread
From: Mika Westerberg @ 2013-05-22 7:58 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Linus Walleij, David Cohen, linux-kernel
On Wed, May 22, 2013 at 10:47:36AM +0300, Andy Shevchenko wrote:
> Otherwise we will end up with traceback from LOCKDEP:
>
> INFO: trying to register non-static key.
> the code is fine but needs lockdep annotation.
> turning off the locking correctness validator.
> CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.10.0-rc2-next-20130521-00028-g09aa9fc #487
> 00000000 00000000 f6c55c54 c1541fe4 f6040bf8 f6c55c8c c1069ef1 c1726bc1
> c1726cc8 c1726c9e 00000000 f6c584e0 f6c58000 f6c55ce8 00000000 f6040bf8
> f6040bf8 00000046 f6c58000 f6c55d00 c106a18d 00000a2b 00000003 00004f02
> Call Trace:
> [<c1541fe4>] dump_stack+0x49/0x77
> [<c1069ef1>] register_lock_class+0x58/0x260
> [<c106a18d>] __lock_acquire+0x94/0xcff
> [<c106adc8>] ? __lock_acquire+0xccf/0xcff
> [<c106b2ad>] lock_acquire+0xcc/0x10d
> [<c1269c7c>] ? lnw_irq_type+0x63/0xe9
> [<c1545da0>] _raw_spin_lock_irqsave+0x32/0x42
> [<c1269c7c>] ? lnw_irq_type+0x63/0xe9
> [<c1269c7c>] lnw_irq_type+0x63/0xe9
> [<c108f454>] __irq_set_trigger+0x98/0x123
> [<c1090225>] irq_set_irq_type+0x2f/0x51
> [<c1090225>] ? irq_set_irq_type+0x2f/0x51
> [<c1269d02>] ? lnw_irq_type+0xe9/0xe9
> [<c1269d34>] lnw_gpio_irq_map+0x32/0x3b
> [<c10914f2>] irq_domain_add_legacy+0xe2/0x107
> [<c1091b53>] irq_domain_add_simple+0x47/0x60
> [<c1269f6e>] lnw_gpio_probe+0x119/0x217
> [<c1271018>] pci_device_probe+0x5a/0x92
> ...
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/4] gpio-langwell: do not use direct access to iomapped memory
2013-05-22 7:47 ` [PATCH v2 2/4] gpio-langwell: do not use direct access to iomapped memory Andy Shevchenko
@ 2013-05-22 7:58 ` Mika Westerberg
0 siblings, 0 replies; 11+ messages in thread
From: Mika Westerberg @ 2013-05-22 7:58 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Linus Walleij, David Cohen, linux-kernel
On Wed, May 22, 2013 at 10:47:37AM +0300, Andy Shevchenko wrote:
> We better to use readl() function instead of bad looking direct access.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/4] gpio-langwell: use managed functions pcim_* and devm_*
2013-05-22 7:47 ` [PATCH v2 3/4] gpio-langwell: use managed functions pcim_* and devm_* Andy Shevchenko
@ 2013-05-22 8:05 ` Mika Westerberg
2013-05-22 8:36 ` Andy Shevchenko
0 siblings, 1 reply; 11+ messages in thread
From: Mika Westerberg @ 2013-05-22 8:05 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Linus Walleij, David Cohen, linux-kernel
On Wed, May 22, 2013 at 10:47:38AM +0300, Andy Shevchenko wrote:
> This makes the error handling much more simpler than open-coding everything and
> in addition makes the probe function smaller an tidier.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
In general this change looks good. Getting rid of 61 lines is certainly an
improvement!
David, are you able to check that this still works on your hardware? (I'm
pretty sure that Andy has tested this already on Medfield)
I have few minor comments, though. See below.
> ---
> drivers/gpio/gpio-langwell.c | 82 ++++++++++++--------------------------------
> 1 file changed, 21 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/gpio/gpio-langwell.c b/drivers/gpio/gpio-langwell.c
> index 8203084..8672282 100644
> --- a/drivers/gpio/gpio-langwell.c
> +++ b/drivers/gpio/gpio-langwell.c
> @@ -320,56 +320,35 @@ static const struct dev_pm_ops lnw_gpio_pm_ops = {
> static int lnw_gpio_probe(struct pci_dev *pdev,
> const struct pci_device_id *id)
> {
> - void __iomem *base;
> - resource_size_t start, len;
> struct lnw_gpio *lnw;
> u32 gpio_base;
> u32 irq_base;
> int retval;
> int ngpio = id->driver_data;
>
> - retval = pci_enable_device(pdev);
> + retval = pcim_enable_device(pdev);
> if (retval)
> return retval;
>
> - retval = pci_request_regions(pdev, "langwell_gpio");
> + retval = pcim_iomap_regions(pdev, 1 << 0 | 1 << 1, pci_name(pdev));
I wonder if "langwell_gpio" is still a better name compared to
pci_name(pdev)?
> if (retval) {
> - dev_err(&pdev->dev, "error requesting resources\n");
> - goto err_pci_req_region;
> - }
> - /* get the gpio_base from bar1 */
> - start = pci_resource_start(pdev, 1);
> - len = pci_resource_len(pdev, 1);
> - base = ioremap_nocache(start, len);
> - if (!base) {
> - dev_err(&pdev->dev, "error mapping bar1\n");
> - retval = -EFAULT;
> - goto err_ioremap;
> + dev_err(&pdev->dev, "I/O memory mapping error\n");
> + return retval;
> }
>
> - irq_base = readl(base);
> - gpio_base = readl(sizeof(u32) + base);
> + irq_base = readl(pcim_iomap_table(pdev)[1]);
> + gpio_base = readl(sizeof(u32) + pcim_iomap_table(pdev)[1]);
Using pcim_iomap_table(pdev)[] is a bit confusing, at least for me. Can you
add a variable where you store that pointer and use that instead?
> /* release the IO mapping, since we already get the info from bar1 */
> - iounmap(base);
> - /* get the register base from bar0 */
> - start = pci_resource_start(pdev, 0);
> - len = pci_resource_len(pdev, 0);
> - base = devm_ioremap_nocache(&pdev->dev, start, len);
> - if (!base) {
> - dev_err(&pdev->dev, "error mapping bar0\n");
> - retval = -EFAULT;
> - goto err_ioremap;
> - }
> + pcim_iounmap_regions(pdev, 1 << 1);
>
> lnw = devm_kzalloc(&pdev->dev, sizeof(*lnw), GFP_KERNEL);
> if (!lnw) {
> dev_err(&pdev->dev, "can't allocate langwell_gpio chip data\n");
> - retval = -ENOMEM;
> - goto err_ioremap;
> + return -ENOMEM;
> }
>
> - lnw->reg_base = base;
> + lnw->reg_base = pcim_iomap_table(pdev)[0];
> lnw->chip.label = dev_name(&pdev->dev);
> lnw->chip.request = lnw_gpio_request;
> lnw->chip.direction_input = lnw_gpio_direction_input;
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 4/4] gpio-langwell: amend error messages
2013-05-22 7:47 ` [PATCH v2 4/4] gpio-langwell: amend error messages Andy Shevchenko
@ 2013-05-22 8:06 ` Mika Westerberg
0 siblings, 0 replies; 11+ messages in thread
From: Mika Westerberg @ 2013-05-22 8:06 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Linus Walleij, David Cohen, linux-kernel
On Wed, May 22, 2013 at 10:47:39AM +0300, Andy Shevchenko wrote:
> There is no need to use hardcoded device name in the error messages, because
> dev_err() prefixes the message with the device name anyway.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/4] gpio-langwell: use managed functions pcim_* and devm_*
2013-05-22 8:05 ` Mika Westerberg
@ 2013-05-22 8:36 ` Andy Shevchenko
2013-05-22 8:50 ` Mika Westerberg
0 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2013-05-22 8:36 UTC (permalink / raw)
To: Mika Westerberg
Cc: Andy Shevchenko, Linus Walleij, David Cohen,
linux-kernel@vger.kernel.org
On Wed, May 22, 2013 at 11:05 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Wed, May 22, 2013 at 10:47:38AM +0300, Andy Shevchenko wrote:
>> This makes the error handling much more simpler than open-coding everything and
>> in addition makes the probe function smaller an tidier.
>>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> In general this change looks good. Getting rid of 61 lines is certainly an
> improvement!
>
> David, are you able to check that this still works on your hardware? (I'm
> pretty sure that Andy has tested this already on Medfield)
I also wonder if it still okay on other platforms where this IP block
is embedded.
> I have few minor comments, though. See below.
Thank you for the review. See my answers below.
>> ---
>> drivers/gpio/gpio-langwell.c | 82 ++++++++++++--------------------------------
>> 1 file changed, 21 insertions(+), 61 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-langwell.c b/drivers/gpio/gpio-langwell.c
>> index 8203084..8672282 100644
>> --- a/drivers/gpio/gpio-langwell.c
>> +++ b/drivers/gpio/gpio-langwell.c
>> @@ -320,56 +320,35 @@ static const struct dev_pm_ops lnw_gpio_pm_ops = {
>> static int lnw_gpio_probe(struct pci_dev *pdev,
>> const struct pci_device_id *id)
>> {
>> - void __iomem *base;
>> - resource_size_t start, len;
>> struct lnw_gpio *lnw;
>> u32 gpio_base;
>> u32 irq_base;
>> int retval;
>> int ngpio = id->driver_data;
>>
>> - retval = pci_enable_device(pdev);
>> + retval = pcim_enable_device(pdev);
>> if (retval)
>> return retval;
>>
>> - retval = pci_request_regions(pdev, "langwell_gpio");
>> + retval = pcim_iomap_regions(pdev, 1 << 0 | 1 << 1, pci_name(pdev));
>
> I wonder if "langwell_gpio" is still a better name compared to
> pci_name(pdev)?
This is used as an internal name for certain resource.
It could be seen in case of using printk("%pR") for example. But even
in that case I prefer to see the actual device as well to which
the resource belongs to.
My general opinion is better to use pci_name(pdev) in the pci drivers instead
of hardcoded pseudo-unique strings.
>> if (retval) {
>> - dev_err(&pdev->dev, "error requesting resources\n");
>> - goto err_pci_req_region;
>> - }
>> - /* get the gpio_base from bar1 */
>> - start = pci_resource_start(pdev, 1);
>> - len = pci_resource_len(pdev, 1);
>> - base = ioremap_nocache(start, len);
>> - if (!base) {
>> - dev_err(&pdev->dev, "error mapping bar1\n");
>> - retval = -EFAULT;
>> - goto err_ioremap;
>> + dev_err(&pdev->dev, "I/O memory mapping error\n");
>> + return retval;
>> }
>>
>> - irq_base = readl(base);
>> - gpio_base = readl(sizeof(u32) + base);
>> + irq_base = readl(pcim_iomap_table(pdev)[1]);
>> + gpio_base = readl(sizeof(u32) + pcim_iomap_table(pdev)[1]);
>
> Using pcim_iomap_table(pdev)[] is a bit confusing, at least for me. Can you
> add a variable where you store that pointer and use that instead?
[Hmm... It returns pointer to an array of pointers.
Okay, I will relive base variable for this as we discussed privately.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/4] gpio-langwell: use managed functions pcim_* and devm_*
2013-05-22 8:36 ` Andy Shevchenko
@ 2013-05-22 8:50 ` Mika Westerberg
0 siblings, 0 replies; 11+ messages in thread
From: Mika Westerberg @ 2013-05-22 8:50 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andy Shevchenko, Linus Walleij, David Cohen,
linux-kernel@vger.kernel.org
On Wed, May 22, 2013 at 11:36:15AM +0300, Andy Shevchenko wrote:
> On Wed, May 22, 2013 at 11:05 AM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Wed, May 22, 2013 at 10:47:38AM +0300, Andy Shevchenko wrote:
> >> This makes the error handling much more simpler than open-coding everything and
> >> in addition makes the probe function smaller an tidier.
> >>
> >> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >
> > In general this change looks good. Getting rid of 61 lines is certainly an
> > improvement!
> >
> > David, are you able to check that this still works on your hardware? (I'm
> > pretty sure that Andy has tested this already on Medfield)
>
> I also wonder if it still okay on other platforms where this IP block
> is embedded.
>
> > I have few minor comments, though. See below.
>
> Thank you for the review. See my answers below.
>
> >> ---
> >> drivers/gpio/gpio-langwell.c | 82 ++++++++++++--------------------------------
> >> 1 file changed, 21 insertions(+), 61 deletions(-)
> >>
> >> diff --git a/drivers/gpio/gpio-langwell.c b/drivers/gpio/gpio-langwell.c
> >> index 8203084..8672282 100644
> >> --- a/drivers/gpio/gpio-langwell.c
> >> +++ b/drivers/gpio/gpio-langwell.c
> >> @@ -320,56 +320,35 @@ static const struct dev_pm_ops lnw_gpio_pm_ops = {
> >> static int lnw_gpio_probe(struct pci_dev *pdev,
> >> const struct pci_device_id *id)
> >> {
> >> - void __iomem *base;
> >> - resource_size_t start, len;
> >> struct lnw_gpio *lnw;
> >> u32 gpio_base;
> >> u32 irq_base;
> >> int retval;
> >> int ngpio = id->driver_data;
> >>
> >> - retval = pci_enable_device(pdev);
> >> + retval = pcim_enable_device(pdev);
> >> if (retval)
> >> return retval;
> >>
> >> - retval = pci_request_regions(pdev, "langwell_gpio");
> >> + retval = pcim_iomap_regions(pdev, 1 << 0 | 1 << 1, pci_name(pdev));
> >
> > I wonder if "langwell_gpio" is still a better name compared to
> > pci_name(pdev)?
>
> This is used as an internal name for certain resource.
>
> It could be seen in case of using printk("%pR") for example. But even
> in that case I prefer to see the actual device as well to which
> the resource belongs to.
>
> My general opinion is better to use pci_name(pdev) in the pci drivers instead
> of hardcoded pseudo-unique strings.
Fair enough :)
> >> if (retval) {
> >> - dev_err(&pdev->dev, "error requesting resources\n");
> >> - goto err_pci_req_region;
> >> - }
> >> - /* get the gpio_base from bar1 */
> >> - start = pci_resource_start(pdev, 1);
> >> - len = pci_resource_len(pdev, 1);
> >> - base = ioremap_nocache(start, len);
> >> - if (!base) {
> >> - dev_err(&pdev->dev, "error mapping bar1\n");
> >> - retval = -EFAULT;
> >> - goto err_ioremap;
> >> + dev_err(&pdev->dev, "I/O memory mapping error\n");
> >> + return retval;
> >> }
> >>
> >> - irq_base = readl(base);
> >> - gpio_base = readl(sizeof(u32) + base);
> >> + irq_base = readl(pcim_iomap_table(pdev)[1]);
> >> + gpio_base = readl(sizeof(u32) + pcim_iomap_table(pdev)[1]);
> >
> > Using pcim_iomap_table(pdev)[] is a bit confusing, at least for me. Can you
> > add a variable where you store that pointer and use that instead?
>
> [Hmm... It returns pointer to an array of pointers.
> Okay, I will relive base variable for this as we discussed privately.
Cool, thanks!
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2013-05-22 8:45 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-05-22 7:47 [PATCH v2 0/4] gpio-langwell: bugfix and amendments Andy Shevchenko
2013-05-22 7:47 ` [PATCH v2 1/4] gpio-langwell: initialize lock before usage Andy Shevchenko
2013-05-22 7:58 ` Mika Westerberg
2013-05-22 7:47 ` [PATCH v2 2/4] gpio-langwell: do not use direct access to iomapped memory Andy Shevchenko
2013-05-22 7:58 ` Mika Westerberg
2013-05-22 7:47 ` [PATCH v2 3/4] gpio-langwell: use managed functions pcim_* and devm_* Andy Shevchenko
2013-05-22 8:05 ` Mika Westerberg
2013-05-22 8:36 ` Andy Shevchenko
2013-05-22 8:50 ` Mika Westerberg
2013-05-22 7:47 ` [PATCH v2 4/4] gpio-langwell: amend error messages Andy Shevchenko
2013-05-22 8:06 ` Mika Westerberg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox