[PATCH 1/6] i2c: ismt: improve usage of devres API

[PATCH 1/6] i2c: ismt: improve usage of devres API

[Andy Shevchenko]

pcim_release() will release any requested region. There is no need to duplicate
this effort in the driver.

Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/i2c/busses/i2c-ismt.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ismt.c b/drivers/i2c/busses/i2c-ismt.c
index f994712..b7a68b5 100644
--- a/drivers/i2c/busses/i2c-ismt.c
+++ b/drivers/i2c/busses/i2c-ismt.c
@@ -904,8 +904,7 @@ ismt_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	priv->smba = pcim_iomap(pdev, SMBBAR, len);
 	if (!priv->smba) {
 		dev_err(&pdev->dev, "Unable to ioremap SMBus BAR\n");
-		err = -ENODEV;
-		goto fail;
+		return -ENODEV;
 	}
 
 	if ((pci_set_dma_mask(pdev, DMA_BIT_MASK(64)) != 0) ||
@@ -915,32 +914,26 @@ ismt_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 						 DMA_BIT_MASK(32)) != 0)) {
 			dev_err(&pdev->dev, "pci_set_dma_mask fail %p\n",
 				pdev);
-			err = -ENODEV;
-			goto fail;
+			return -ENODEV;
 		}
 	}
 
 	err = ismt_dev_init(priv);
 	if (err)
-		goto fail;
+		return err;
 
 	ismt_hw_init(priv);
 
 	err = ismt_int_init(priv);
 	if (err)
-		goto fail;
+		return err;
 
 	err = i2c_add_adapter(&priv->adapter);
 	if (err) {
 		dev_err(&pdev->dev, "Failed to add SMBus iSMT adapter\n");
-		err = -ENODEV;
-		goto fail;
+		return -ENODEV;
 	}
 	return 0;
-
-fail:
-	pci_release_region(pdev, SMBBAR);
-	return err;
 }
 
 /**
@@ -952,7 +945,6 @@ static void ismt_remove(struct pci_dev *pdev)
 	struct ismt_priv *priv = pci_get_drvdata(pdev);
 
 	i2c_del_adapter(&priv->adapter);
-	pci_release_region(pdev, SMBBAR);
 }
 
 /**
-- 
2.5.1

[PATCH 2/6] i2c: ismt: PCI core handles power state for us

[Andy Shevchenko]

There is no need to repeat the work that is already done in the PCI driver
core. Remove suspend and resume callbacks.

Note that there is no more calls performed to enable or disable a PCI
device during suspend-resume cycle. Nowadays they seems to be
superflous. Someone can read more in [1].

[1] https://www.kernel.org/doc/ols/2009/ols2009-pages-319-330.pdf

Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/i2c/busses/i2c-ismt.c | 33 ---------------------------------
 1 file changed, 33 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ismt.c b/drivers/i2c/busses/i2c-ismt.c
index b7a68b5..570b0a0 100644
--- a/drivers/i2c/busses/i2c-ismt.c
+++ b/drivers/i2c/busses/i2c-ismt.c
@@ -947,44 +947,11 @@ static void ismt_remove(struct pci_dev *pdev)
 	i2c_del_adapter(&priv->adapter);
 }
 
-/**
- * ismt_suspend() - place the device in suspend
- * @pdev: PCI-Express device
- * @mesg: PM message
- */
-#ifdef CONFIG_PM
-static int ismt_suspend(struct pci_dev *pdev, pm_message_t mesg)
-{
-	pci_save_state(pdev);
-	pci_set_power_state(pdev, pci_choose_state(pdev, mesg));
-	return 0;
-}
-
-/**
- * ismt_resume() - PCI resume code
- * @pdev: PCI-Express device
- */
-static int ismt_resume(struct pci_dev *pdev)
-{
-	pci_set_power_state(pdev, PCI_D0);
-	pci_restore_state(pdev);
-	return pci_enable_device(pdev);
-}
-
-#else
-
-#define ismt_suspend NULL
-#define ismt_resume NULL
-
-#endif
-
 static struct pci_driver ismt_driver = {
 	.name = "ismt_smbus",
 	.id_table = ismt_ids,
 	.probe = ismt_probe,
 	.remove = ismt_remove,
-	.suspend = ismt_suspend,
-	.resume = ismt_resume,
 };
 
 module_pci_driver(ismt_driver);
-- 
2.5.1

Re: [PATCH 2/6] i2c: ismt: PCI core handles power state for us

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 740 bytes --]

On Wed, Sep 16, 2015 at 05:23:59PM +0300, Andy Shevchenko wrote:
> There is no need to repeat the work that is already done in the PCI driver
> core. Remove suspend and resume callbacks.
> 
> Note that there is no more calls performed to enable or disable a PCI
> device during suspend-resume cycle. Nowadays they seems to be
> superflous. Someone can read more in [1].
> 
> [1] https://www.kernel.org/doc/ols/2009/ols2009-pages-319-330.pdf
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Checkpatch said:

WARNING: 'superflous' may be misspelled - perhaps 'superfluous'?
#6: 
superflous. Someone can read more in [1].

total: 0 errors, 1 warnings, 0 checks, 44 lines checked

Fixed it here.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 3/6] i2c: ismt: do not duplicate msi_enabled flag

[Andy Shevchenko]

struct pci_dev already has a flag to track if MSI is enabled or not. Use it
directly.

Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/i2c/busses/i2c-ismt.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ismt.c b/drivers/i2c/busses/i2c-ismt.c
index 570b0a0..6c2058d 100644
--- a/drivers/i2c/busses/i2c-ismt.c
+++ b/drivers/i2c/busses/i2c-ismt.c
@@ -172,7 +172,6 @@ struct ismt_priv {
 	u8 head;				/* ring buffer head pointer */
 	struct completion cmp;			/* interrupt completion */
 	u8 dma_buffer[I2C_SMBUS_BLOCK_MAX + 1];	/* temp R/W data buffer */
-	bool using_msi;				/* type of interrupt flag */
 };
 
 /**
@@ -398,7 +397,7 @@ static int ismt_access(struct i2c_adapter *adap, u16 addr,
 	desc->tgtaddr_rw = ISMT_DESC_ADDR_RW(addr, read_write);
 
 	/* Initialize common control bits */
-	if (likely(priv->using_msi))
+	if (likely(pci_dev_msi_enabled(priv->pci_dev)))
 		desc->control = ISMT_DESC_INT | ISMT_DESC_FAIR;
 	else
 		desc->control = ISMT_DESC_FAIR;
@@ -806,7 +805,6 @@ static int ismt_int_init(struct ismt_priv *priv)
 		goto intx;
 	}
 
-	priv->using_msi = true;
 	goto done;
 
 	/* Try using legacy interrupts */
@@ -822,8 +820,6 @@ intx:
 		return -ENODEV;
 	}
 
-	priv->using_msi = false;
-
 done:
 	return 0;
 }
-- 
2.5.1

[PATCH 4/6] i2c: ismt: propagate actual error code

[Andy Shevchenko]

Propagate actual return code when requesting interrupt fails.

Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/i2c/busses/i2c-ismt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-ismt.c b/drivers/i2c/busses/i2c-ismt.c
index 6c2058d..3bc4bad 100644
--- a/drivers/i2c/busses/i2c-ismt.c
+++ b/drivers/i2c/busses/i2c-ismt.c
@@ -817,7 +817,7 @@ intx:
 			       priv);
 	if (err) {
 		dev_err(&priv->pci_dev->dev, "no usable interrupts\n");
-		return -ENODEV;
+		return err;
 	}
 
 done:
-- 
2.5.1

[PATCH 5/6] i2c: ismt: issue a warning when fail to request MSI

[Andy Shevchenko]

Issue the warning in all error paths when unable to register MSI or its
handler.

Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/i2c/busses/i2c-ismt.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ismt.c b/drivers/i2c/busses/i2c-ismt.c
index 3bc4bad..48e0693 100644
--- a/drivers/i2c/busses/i2c-ismt.c
+++ b/drivers/i2c/busses/i2c-ismt.c
@@ -788,11 +788,8 @@ static int ismt_int_init(struct ismt_priv *priv)
 
 	/* Try using MSI interrupts */
 	err = pci_enable_msi(priv->pci_dev);
-	if (err) {
-		dev_warn(&priv->pci_dev->dev,
-			 "Unable to use MSI interrupts, falling back to legacy\n");
+	if (err)
 		goto intx;
-	}
 
 	err = devm_request_irq(&priv->pci_dev->dev,
 			       priv->pci_dev->irq,
@@ -805,10 +802,13 @@ static int ismt_int_init(struct ismt_priv *priv)
 		goto intx;
 	}
 
-	goto done;
+	return 0;
 
 	/* Try using legacy interrupts */
 intx:
+	dev_warn(&priv->pci_dev->dev,
+		 "Unable to use MSI interrupts, falling back to legacy\n");
+
 	err = devm_request_irq(&priv->pci_dev->dev,
 			       priv->pci_dev->irq,
 			       ismt_do_interrupt,
@@ -820,7 +820,6 @@ intx:
 		return err;
 	}
 
-done:
 	return 0;
 }
 
-- 
2.5.1

[PATCH 6/6] i2c: ismt: mark register space with __iomem

[Andy Shevchenko]

This fixes the code to suppress sparse warnings like:

drivers/i2c/busses/i2c-ismt.c:725:36: warning: incorrect type in argument 2 (different address spaces)
drivers/i2c/busses/i2c-ismt.c:725:36:    expected void volatile [noderef] <asn:2>*addr
drivers/i2c/busses/i2c-ismt.c:725:36:    got void *

Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/i2c/busses/i2c-ismt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-ismt.c b/drivers/i2c/busses/i2c-ismt.c
index 48e0693..80648be 100644
--- a/drivers/i2c/busses/i2c-ismt.c
+++ b/drivers/i2c/busses/i2c-ismt.c
@@ -165,7 +165,7 @@ struct ismt_desc {
 
 struct ismt_priv {
 	struct i2c_adapter adapter;
-	void *smba;				/* PCI BAR */
+	void __iomem *smba;			/* PCI BAR */
 	struct pci_dev *pci_dev;
 	struct ismt_desc *hw;			/* descriptor virt base addr */
 	dma_addr_t io_rng_dma;			/* descriptor HW base addr */
-- 
2.5.1

Re: [PATCH 1/6] i2c: ismt: improve usage of devres API

[Andy Shevchenko]

On Wed, 2015-09-16 at 17:23 +0300, Andy Shevchenko wrote:
> pcim_release() will release any requested region. There is no need to 
> duplicate
> this effort in the driver.

Any comments on that clean up series?

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/i2c/busses/i2c-ismt.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-ismt.c b/drivers/i2c/busses/i2c
> -ismt.c
> index f994712..b7a68b5 100644
> --- a/drivers/i2c/busses/i2c-ismt.c
> +++ b/drivers/i2c/busses/i2c-ismt.c
> @@ -904,8 +904,7 @@ ismt_probe(struct pci_dev *pdev, const struct 
> pci_device_id *id)
>  	priv->smba = pcim_iomap(pdev, SMBBAR, len);
>  	if (!priv->smba) {
>  		dev_err(&pdev->dev, "Unable to ioremap SMBus 
> BAR\n");
> -		err = -ENODEV;
> -		goto fail;
> +		return -ENODEV;
>  	}
>  
>  	if ((pci_set_dma_mask(pdev, DMA_BIT_MASK(64)) != 0) ||
> @@ -915,32 +914,26 @@ ismt_probe(struct pci_dev *pdev, const struct 
> pci_device_id *id)
>  						 DMA_BIT_MASK(32)) 
> != 0)) {
>  			dev_err(&pdev->dev, "pci_set_dma_mask fail 
> %p\n",
>  				pdev);
> -			err = -ENODEV;
> -			goto fail;
> +			return -ENODEV;
>  		}
>  	}
>  
>  	err = ismt_dev_init(priv);
>  	if (err)
> -		goto fail;
> +		return err;
>  
>  	ismt_hw_init(priv);
>  
>  	err = ismt_int_init(priv);
>  	if (err)
> -		goto fail;
> +		return err;
>  
>  	err = i2c_add_adapter(&priv->adapter);
>  	if (err) {
>  		dev_err(&pdev->dev, "Failed to add SMBus iSMT 
> adapter\n");
> -		err = -ENODEV;
> -		goto fail;
> +		return -ENODEV;
>  	}
>  	return 0;
> -
> -fail:
> -	pci_release_region(pdev, SMBBAR);
> -	return err;
>  }
>  
>  /**
> @@ -952,7 +945,6 @@ static void ismt_remove(struct pci_dev *pdev)
>  	struct ismt_priv *priv = pci_get_drvdata(pdev);
>  
>  	i2c_del_adapter(&priv->adapter);
> -	pci_release_region(pdev, SMBBAR);
>  }
>  
>  /**

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 1/6] i2c: ismt: improve usage of devres API

[Mika Westerberg]

On Mon, Sep 28, 2015 at 01:45:32PM +0300, Andy Shevchenko wrote:
> On Wed, 2015-09-16 at 17:23 +0300, Andy Shevchenko wrote:
> > pcim_release() will release any requested region. There is no need to 
> > duplicate
> > this effort in the driver.
> 
> Any comments on that clean up series?

FWIW the whole series looks good to me,

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH 1/6] i2c: ismt: improve usage of devres API

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 532 bytes --]

On Mon, Sep 28, 2015 at 02:01:54PM +0300, Mika Westerberg wrote:
> On Mon, Sep 28, 2015 at 01:45:32PM +0300, Andy Shevchenko wrote:
> > On Wed, 2015-09-16 at 17:23 +0300, Andy Shevchenko wrote:
> > > pcim_release() will release any requested region. There is no need to 
> > > duplicate
> > > this effort in the driver.
> > 
> > Any comments on that clean up series?
> 
> FWIW the whole series looks good to me,
> 
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Applied all to for-next, thanks!



[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]