[PATCHv3 1/5] i2c: i801: Don't break user-visible strings

[PATCHv3 1/5] i2c: i801: Don't break user-visible strings

[Jarkko Nikula]

It makes more difficult to grep these error prints from sources if they are
split to multiple source lines.

Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Reviewed-by: Jean Delvare <jdelvare-l3A5Bk7waGM@public.gmane.org>
---
 drivers/i2c/busses/i2c-i801.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 8fafb254e42a..7d1f4a478c54 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1192,8 +1192,8 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	/* Determine the address of the SMBus area */
 	priv->smba = pci_resource_start(dev, SMBBAR);
 	if (!priv->smba) {
-		dev_err(&dev->dev, "SMBus base address uninitialized, "
-			"upgrade BIOS\n");
+		dev_err(&dev->dev,
+			"SMBus base address uninitialized, upgrade BIOS\n");
 		err = -ENODEV;
 		goto exit;
 	}
@@ -1206,8 +1206,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 	err = pci_request_region(dev, SMBBAR, i801_driver.name);
 	if (err) {
-		dev_err(&dev->dev, "Failed to request SMBus region "
-			"0x%lx-0x%Lx\n", priv->smba,
+		dev_err(&dev->dev,
+			"Failed to request SMBus region 0x%lx-0x%Lx\n",
+			priv->smba,
 			(unsigned long long)pci_resource_end(dev, SMBBAR));
 		goto exit;
 	}
-- 
2.1.4

[PATCHv3 2/5] i2c: i801: Remove i801_driver forward declaration

[Jarkko Nikula]

struct pci_driver i801_driver forward declaration is needed only for
accessing the name field. Remove it and use dev_driver_string() instead.

Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Reviewed-by: Jean Delvare <jdelvare-l3A5Bk7waGM@public.gmane.org>
---
 drivers/i2c/busses/i2c-i801.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 7d1f4a478c54..5f827dfc671a 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -223,8 +223,6 @@ struct i801_priv {
 #endif
 };
 
-static struct pci_driver i801_driver;
-
 #define FEATURE_SMBUS_PEC	(1 << 0)
 #define FEATURE_BLOCK_BUFFER	(1 << 1)
 #define FEATURE_BLOCK_PROC	(1 << 2)
@@ -1204,7 +1202,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		goto exit;
 	}
 
-	err = pci_request_region(dev, SMBBAR, i801_driver.name);
+	err = pci_request_region(dev, SMBBAR, dev_driver_string(&dev->dev));
 	if (err) {
 		dev_err(&dev->dev,
 			"Failed to request SMBus region 0x%lx-0x%Lx\n",
@@ -1256,7 +1254,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		init_waitqueue_head(&priv->waitq);
 
 		err = request_irq(dev->irq, i801_isr, IRQF_SHARED,
-				  i801_driver.name, priv);
+				  dev_driver_string(&dev->dev), priv);
 		if (err) {
 			dev_err(&dev->dev, "Failed to allocate irq %d: %d\n",
 				dev->irq, err);
-- 
2.1.4

[PATCHv3 3/5] i2c: i801: Use managed devm_* memory and irq allocation

[Jarkko Nikula]

This simplifies the error and remove paths.

Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Reviewed-by: Jean Delvare <jdelvare-l3A5Bk7waGM@public.gmane.org>
---
 drivers/i2c/busses/i2c-i801.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 5f827dfc671a..b1d725d758bb 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1138,7 +1138,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	int err, i;
 	struct i801_priv *priv;
 
-	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	priv = devm_kzalloc(&dev->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
@@ -1253,8 +1253,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	if (priv->features & FEATURE_IRQ) {
 		init_waitqueue_head(&priv->waitq);
 
-		err = request_irq(dev->irq, i801_isr, IRQF_SHARED,
-				  dev_driver_string(&dev->dev), priv);
+		err = devm_request_irq(&dev->dev, dev->irq, i801_isr,
+				       IRQF_SHARED,
+				       dev_driver_string(&dev->dev), priv);
 		if (err) {
 			dev_err(&dev->dev, "Failed to allocate irq %d: %d\n",
 				dev->irq, err);
@@ -1275,7 +1276,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	err = i2c_add_adapter(&priv->adapter);
 	if (err) {
 		dev_err(&dev->dev, "Failed to add SMBus adapter\n");
-		goto exit_free_irq;
+		goto exit_release;
 	}
 
 	i801_probe_optional_slaves(priv);
@@ -1286,12 +1287,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 	return 0;
 
-exit_free_irq:
-	if (priv->features & FEATURE_IRQ)
-		free_irq(dev->irq, priv);
+exit_release:
 	pci_release_region(dev, SMBBAR);
 exit:
-	kfree(priv);
 	return err;
 }
 
@@ -1303,11 +1301,8 @@ static void i801_remove(struct pci_dev *dev)
 	i2c_del_adapter(&priv->adapter);
 	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
 
-	if (priv->features & FEATURE_IRQ)
-		free_irq(dev->irq, priv);
 	pci_release_region(dev, SMBBAR);
 
-	kfree(priv);
 	/*
 	 * do not call pci_disable_device(dev) since it can cause hard hangs on
 	 * some systems during power-off (eg. Fujitsu-Siemens Lifebook E8010)
-- 
2.1.4

[PATCHv3 4/5] i2c: i801: Remove pci_enable_device() call from i801_resume()

[Jarkko Nikula]

Since pci_disable_device() is not called from i801_suspend() and power
state is set already it means that subsequent pci_enable_device() calls do
practically nothing but monotonically increase struct pci_dev enable_cnt.

Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/i2c/busses/i2c-i801.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index b1d725d758bb..5fb35464f693 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1324,7 +1324,7 @@ static int i801_resume(struct pci_dev *dev)
 {
 	pci_set_power_state(dev, PCI_D0);
 	pci_restore_state(dev);
-	return pci_enable_device(dev);
+	return 0;
 }
 #else
 #define i801_suspend NULL
-- 
2.1.4

Re: [PATCHv3 4/5] i2c: i801: Remove pci_enable_device() call from i801_resume()

[Jean Delvare]

On Fri, 13 Feb 2015 15:52:24 +0200, Jarkko Nikula wrote:
> Since pci_disable_device() is not called from i801_suspend() and power
> state is set already it means that subsequent pci_enable_device() calls do
> practically nothing but monotonically increase struct pci_dev enable_cnt.
> 
> Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-i801.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index b1d725d758bb..5fb35464f693 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1324,7 +1324,7 @@ static int i801_resume(struct pci_dev *dev)
>  {
>  	pci_set_power_state(dev, PCI_D0);
>  	pci_restore_state(dev);
> -	return pci_enable_device(dev);
> +	return 0;
>  }
>  #else
>  #define i801_suspend NULL

Reviewed-by: Jean Delvare <jdelvare-l3A5Bk7waGM@public.gmane.org>

-- 
Jean Delvare
SUSE L3 Support

[PATCHv3 5/5] i2c: i801: Use managed pcim_* PCI device initialization and reservation

[Jarkko Nikula]

Simplifies the code a bit and makes easier to disable PCI device on driver
detach by removing the pcim_pin_device() call in the future if needed.

Reason why i2c-i801.c doesn't ever call pci_disable_device() was because it
made some systems to hang during power-off. See commit d6fcb3b9cf77
("[PATCH] i2c-i801.c: don't pci_disable_device() after it was just enabled")
and
http://marc.info/?l=linux-kernel&m=115160053309535&w=2

Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
Changes from v2:
- over 80 characters long pcim_iomap_regions line splitted
- gotos and error labels removed
---
 drivers/i2c/busses/i2c-i801.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 5fb35464f693..5ecbb3fdc27e 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1180,35 +1180,35 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	}
 	priv->features &= ~disable_features;
 
-	err = pci_enable_device(dev);
+	err = pcim_enable_device(dev);
 	if (err) {
 		dev_err(&dev->dev, "Failed to enable SMBus PCI device (%d)\n",
 			err);
-		goto exit;
+		return err;
 	}
+	pcim_pin_device(dev);
 
 	/* Determine the address of the SMBus area */
 	priv->smba = pci_resource_start(dev, SMBBAR);
 	if (!priv->smba) {
 		dev_err(&dev->dev,
 			"SMBus base address uninitialized, upgrade BIOS\n");
-		err = -ENODEV;
-		goto exit;
+		return -ENODEV;
 	}
 
 	err = acpi_check_resource_conflict(&dev->resource[SMBBAR]);
 	if (err) {
-		err = -ENODEV;
-		goto exit;
+		return -ENODEV;
 	}
 
-	err = pci_request_region(dev, SMBBAR, dev_driver_string(&dev->dev));
+	err = pcim_iomap_regions(dev, 1 << SMBBAR,
+				 dev_driver_string(&dev->dev));
 	if (err) {
 		dev_err(&dev->dev,
 			"Failed to request SMBus region 0x%lx-0x%Lx\n",
 			priv->smba,
 			(unsigned long long)pci_resource_end(dev, SMBBAR));
-		goto exit;
+		return err;
 	}
 
 	pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &temp);
@@ -1276,7 +1276,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	err = i2c_add_adapter(&priv->adapter);
 	if (err) {
 		dev_err(&dev->dev, "Failed to add SMBus adapter\n");
-		goto exit_release;
+		return err;
 	}
 
 	i801_probe_optional_slaves(priv);
@@ -1286,11 +1286,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	pci_set_drvdata(dev, priv);
 
 	return 0;
-
-exit_release:
-	pci_release_region(dev, SMBBAR);
-exit:
-	return err;
 }
 
 static void i801_remove(struct pci_dev *dev)
@@ -1301,8 +1296,6 @@ static void i801_remove(struct pci_dev *dev)
 	i2c_del_adapter(&priv->adapter);
 	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
 
-	pci_release_region(dev, SMBBAR);
-
 	/*
 	 * do not call pci_disable_device(dev) since it can cause hard hangs on
 	 * some systems during power-off (eg. Fujitsu-Siemens Lifebook E8010)
-- 
2.1.4

Re: [PATCHv3 5/5] i2c: i801: Use managed pcim_* PCI device initialization and reservation

[Jean Delvare]

On Fri, 13 Feb 2015 15:52:25 +0200, Jarkko Nikula wrote:
> Simplifies the code a bit and makes easier to disable PCI device on driver
> detach by removing the pcim_pin_device() call in the future if needed.
> 
> Reason why i2c-i801.c doesn't ever call pci_disable_device() was because it
> made some systems to hang during power-off. See commit d6fcb3b9cf77
> ("[PATCH] i2c-i801.c: don't pci_disable_device() after it was just enabled")
> and
> http://marc.info/?l=linux-kernel&m=115160053309535&w=2
> 
> Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
> Changes from v2:
> - over 80 characters long pcim_iomap_regions line splitted
> - gotos and error labels removed
> ---
>  drivers/i2c/busses/i2c-i801.c | 25 +++++++++----------------
>  1 file changed, 9 insertions(+), 16 deletions(-)
> (...)

Reviewed-by: Jean Delvare <jdelvare-l3A5Bk7waGM@public.gmane.org>

Wolfram, please commit this series, I have reviewed all patches and
tested the updated driver successfully on 3 different machines.

Thanks Jarkko for the good work.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCHv3 5/5] i2c: i801: Use managed pcim_* PCI device initialization and reservation

[Wolfram Sang]

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

On Mon, Feb 16, 2015 at 10:29:18AM +0100, Jean Delvare wrote:
> On Fri, 13 Feb 2015 15:52:25 +0200, Jarkko Nikula wrote:
> > Simplifies the code a bit and makes easier to disable PCI device on driver
> > detach by removing the pcim_pin_device() call in the future if needed.
> > 
> > Reason why i2c-i801.c doesn't ever call pci_disable_device() was because it
> > made some systems to hang during power-off. See commit d6fcb3b9cf77
> > ("[PATCH] i2c-i801.c: don't pci_disable_device() after it was just enabled")
> > and
> > http://marc.info/?l=linux-kernel&m=115160053309535&w=2
> > 
> > Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > ---
> > Changes from v2:
> > - over 80 characters long pcim_iomap_regions line splitted
> > - gotos and error labels removed
> > ---
> >  drivers/i2c/busses/i2c-i801.c | 25 +++++++++----------------
> >  1 file changed, 9 insertions(+), 16 deletions(-)
> > (...)
> 
> Reviewed-by: Jean Delvare <jdelvare-l3A5Bk7waGM@public.gmane.org>
> 
> Wolfram, please commit this series, I have reviewed all patches and
> tested the updated driver successfully on 3 different machines.
> 
> Thanks Jarkko for the good work.

Applied to for-next, thanks!


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