i2c: i801: Don't break user-visible strings

PATCH Applied Jarkko Nikula 3 participants 8 messages Feb 13, 2015 – Mar 6, 2015 4 messages not in the archive

Subsystems (2)

I2C subsystem host drivers I2C/smbus controller drivers for pc

lore

Share & tools

lore

Jarkko Nikula

author

2015-02-13 13:52 UTC #0 lore raw

PATCH v3 1/5 i2c: i801: Don't break user-visible strings

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/[email protected]>

Reviewed-by Jean Delvare <[email protected]>

---
 drivers/i2c/busses/i2c-i801.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

drivers/i2c/busses/i2c-i801.c +5 -4

index 8fafb254e42a..7d1f4a478c54 100644
@@ -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;
 	}

Signature · 1 line

2.1.4

A message that is not in the archive (sent off-list or not yet received): 1423835545-17991-1-git-send-email-jarkko.nikula-VuQAYsv1563Yd54FQh9/[email protected]

Jarkko Nikula

author

2015-02-13 13:52 UTC #2 lore raw

PATCH v3 2/5 i2c: i801: Remove i801_driver forward declaration

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/[email protected]>

Reviewed-by Jean Delvare <[email protected]>

---
 drivers/i2c/busses/i2c-i801.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

drivers/i2c/busses/i2c-i801.c +2 -4

index 7d1f4a478c54..5f827dfc671a 100644
@@ -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);

Signature · 1 line

2.1.4

Jarkko Nikula

author

2015-02-13 13:52 UTC #3 lore raw

PATCH v3 3/5 i2c: i801: Use managed devm_* memory and irq allocation

In reply to a message not in the archive

This simplifies the error and remove paths.

Signed-off-by Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/[email protected]>

Reviewed-by Jean Delvare <[email protected]>

---
 drivers/i2c/busses/i2c-i801.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

drivers/i2c/busses/i2c-i801.c +6 ~~-11~~

index 5f827dfc671a..b1d725d758bb 100644
@@ -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)

Signature · 1 line

2.1.4

Jarkko Nikula

author

2015-02-13 13:52 UTC #4 lore raw

PATCH v3 4/5 i2c: i801: Remove pci_enable_device() call from i801_resume()

In reply to a message not in the archive

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/[email protected]>

---
 drivers/i2c/busses/i2c-i801.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/i2c/busses/i2c-i801.c +1 -1

index b1d725d758bb..5fb35464f693 100644
@@ -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

Signature · 1 line

2.1.4

A message that is not in the archive (sent off-list or not yet received): 1423835545-17991-4-git-send-email-jarkko.nikula-VuQAYsv1563Yd54FQh9/[email protected]

Jean Delvare

Reviewed-by

2015-02-16 09:27 UTC #6 lore raw

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

Quoted 14 lines · 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/[email protected]>
---
 drivers/i2c/busses/i2c-i801.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/busses/i2c-i801.c +0 -0
index b1d725d758bb..5fb35464f693 100644

drivers/i2c/busses/i2c-i801.c static int i801_resume(struct pci_dev *dev) · line 1330

Quoted 8 lines

drivers/i2c/busses/i2c-i801.c +1 -1

@@ -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 <[email protected]>

Signature · 2 lines

Jean Delvare
SUSE L3 Support

Jarkko Nikula

author

2015-02-13 13:52 UTC #7 lore raw

PATCH v3 5/5 i2c: i801: Use managed pcim_* PCI device initialization and reservation

In reply to a message not in the archive

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/[email protected]>

---
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(-)

drivers/i2c/busses/i2c-i801.c +9 ~~-16~~

index 5fb35464f693..5ecbb3fdc27e 100644
@@ -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)

Signature · 1 line

2.1.4

A message that is not in the archive (sent off-list or not yet received): 1423835545-17991-5-git-send-email-jarkko.nikula-VuQAYsv1563Yd54FQh9/[email protected]

Jean Delvare

Reviewed-by

2015-02-16 09:29 UTC #9 lore raw

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

Quoted 10 lines · 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/[email protected]>

Quoted 8 lines

---
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 <[email protected]>

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.

Signature · 2 lines

Jean Delvare
SUSE L3 Support

A message that is not in the archive (sent off-list or not yet received): [email protected]

Wolfram Sang

Applied

2015-03-06 23:51 UTC #11 lore raw

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

Quoted 20 lines · Jean Delvare

Quoted 10 lines · 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/[email protected]>
Quoted 8 lines
---
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(-)
(...)

Quoted 6 lines

Reviewed-by Jean Delvare <[email protected]>
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!