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

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

[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

[PATCH v2 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

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

[Mika Westerberg]

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>

[PATCH v2 2/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 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

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

[Mika Westerberg]

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>

[PATCH v2 3/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 | 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

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

[Mika Westerberg]

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;

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

[Andy Shevchenko]

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

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

[Mika Westerberg]

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!

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

[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

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

[Mika Westerberg]

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>