[PATCH 0/4] gpio-langwell: bugfix and amendments

[PATCH 0/4] gpio-langwell: bugfix and amendments

[Andy Shevchenko]

There is locking bug fix and few amendments. Each commit message verbose
enoough I think.

Andy Shevchenko (4):
  gpio-langwell: initialize lock before usage
  gpio-langwell: amend error messages
  gpio-langwell: do not use direct access to iomapped memory
  gpio-langwell: use managed functions pcim_* and devm_*

 drivers/gpio/gpio-langwell.c | 93 +++++++++++++-------------------------------
 1 file changed, 28 insertions(+), 65 deletions(-)

-- 
1.8.2.rc0.22.gb3600c3

[PATCH 1/4] gpio-langwell: initialize lock before usage

[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

[PATCH 2/4] gpio-langwell: amend error messages

[Andy Shevchenko]

Instead of hardcoded names let's use __func__ macro.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-langwell.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-langwell.c b/drivers/gpio/gpio-langwell.c
index c1f33dd..ab797cc 100644
--- a/drivers/gpio/gpio-langwell.c
+++ b/drivers/gpio/gpio-langwell.c
@@ -392,7 +392,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, "%s error %d\n", __func__, retval);
 		goto err_ioremap;
 	}
 
@@ -459,8 +459,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, "%s error %d\n", __func__, retval);
 		goto err_ioremap;
 	}
 	platform_set_drvdata(pdev, lnw);
-- 
1.8.2.rc0.22.gb3600c3

[PATCH 3/4] gpio-langwell: do not use direct access to iomapped memory

[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 ab797cc..652c1c7 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

[PATCH 4/4] gpio-langwell: use managed functions pcim_* and devm_*

[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 | 78 ++++++++++++--------------------------------
 1 file changed, 20 insertions(+), 58 deletions(-)

diff --git a/drivers/gpio/gpio-langwell.c b/drivers/gpio/gpio-langwell.c
index 652c1c7..bdb24ef 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;
@@ -388,14 +367,14 @@ static int lnw_gpio_probe(struct pci_dev *pdev,
 					    &lnw_gpio_irq_ops, lnw);
 	if (!lnw->domain) {
 		retval = -ENOMEM;
-		goto err_ioremap;
+		return retval;
 	}
 
 	pci_set_drvdata(pdev, lnw);
 	retval = gpiochip_add(&lnw->chip);
 	if (retval) {
 		dev_err(&pdev->dev, "%s error %d\n", __func__, retval);
-		goto err_ioremap;
+		return retval;
 	}
 
 	lnw_irq_init_hw(lnw);
@@ -407,12 +386,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 +403,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);
@@ -462,15 +432,10 @@ static int wp_gpio_probe(struct platform_device *pdev)
 	retval = gpiochip_add(gc);
 	if (retval) {
 		dev_err(&pdev->dev, "%s error %d\n", __func__, 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)
@@ -480,9 +445,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

Re: [PATCH 0/4] gpio-langwell: bugfix and amendments

[David Cohen]

Hi Andy,

Thanks for the patches. :)

On 05/21/2013 03:33 AM, Andy Shevchenko wrote:
> There is locking bug fix and few amendments. Each commit message verbose
> enoough I think.
>
> Andy Shevchenko (4):
>    gpio-langwell: initialize lock before usage
>    gpio-langwell: amend error messages
>    gpio-langwell: do not use direct access to iomapped memory
>    gpio-langwell: use managed functions pcim_* and devm_*

I wonder if you should rebase those patches on top of this one:
https://patchwork.kernel.org/patch/2527431/

Maybe Linux Walleij knows better.

Br, David Cohen

>   drivers/gpio/gpio-langwell.c | 93 +++++++++++++-------------------------------
>   1 file changed, 28 insertions(+), 65 deletions(-)
>

Re: [PATCH 0/4] gpio-langwell: bugfix and amendments

[Andy Shevchenko]

On Tue, May 21, 2013 at 8:23 PM, David Cohen <david.a.cohen@intel.com> wrote:
> On 05/21/2013 03:33 AM, Andy Shevchenko wrote:
>>
>> There is locking bug fix and few amendments. Each commit message verbose
>> enoough I think.
>>
>> Andy Shevchenko (4):
>>    gpio-langwell: initialize lock before usage
>>    gpio-langwell: amend error messages
>>    gpio-langwell: do not use direct access to iomapped memory
>>    gpio-langwell: use managed functions pcim_* and devm_*
>
>
> I wonder if you should rebase those patches on top of this one:
> https://patchwork.kernel.org/patch/2527431/

I actually made my patchset on top of that one.
I think I have to fix patch 2/4 here to call that as real amendment.

--
With Best Regards,
Andy Shevchenko