[PATCH] i2c: i801: add helper i801_restore_regs

[PATCH] i2c: i801: add helper i801_restore_regs

[Heiner Kallweit]

In few places relevant registers are reset to their initial value on
driver load. Factor this out to new helper i801_restore_regs to avoid
code duplication.
Even though no actual problems are known, this patch may contribute
to avoiding potential issues by:
- restoring register values also in the error path of i2c_add_adapter
- making restoring registers the last step (especially in i801_remove)

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
This patch is a reworked version of a part of previously discussed patch
"i2c: i801: fix cleanup code in remove() and error path of probe()".
---
 drivers/i2c/busses/i2c-i801.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 9bd712eaf..811541797 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1629,6 +1629,12 @@ static void i801_setup_hstcfg(struct i801_priv *priv)
 	pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hstcfg);
 }
 
+static void i801_restore_regs(struct i801_priv *priv)
+{
+	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
+	pci_write_config_byte(priv->pci_dev, SMBHSTCFG, priv->original_hstcfg);
+}
+
 static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	int err, i;
@@ -1755,6 +1761,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	if (err) {
 		platform_device_unregister(priv->tco_pdev);
 		i801_acpi_remove(priv);
+		i801_restore_regs(priv);
 		return err;
 	}
 
@@ -1779,12 +1786,10 @@ static void i801_remove(struct pci_dev *dev)
 {
 	struct i801_priv *priv = pci_get_drvdata(dev);
 
-	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
 	i801_disable_host_notify(priv);
 	i801_del_mux(priv);
 	i2c_del_adapter(&priv->adapter);
 	i801_acpi_remove(priv);
-	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
 
 	platform_device_unregister(priv->tco_pdev);
 
@@ -1792,6 +1797,8 @@ static void i801_remove(struct pci_dev *dev)
 	if (!priv->acpi_reserved)
 		pm_runtime_get_noresume(&dev->dev);
 
+	i801_restore_regs(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)
@@ -1802,18 +1809,17 @@ static void i801_shutdown(struct pci_dev *dev)
 {
 	struct i801_priv *priv = pci_get_drvdata(dev);
 
-	/* Restore config registers to avoid hard hang on some systems */
-	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
 	i801_disable_host_notify(priv);
-	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
+	/* Restore config registers to avoid hard hang on some systems */
+	i801_restore_regs(priv);
 }
 
 static int i801_suspend(struct device *dev)
 {
 	struct i801_priv *priv = dev_get_drvdata(dev);
 
-	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
-	pci_write_config_byte(priv->pci_dev, SMBHSTCFG, priv->original_hstcfg);
+	i801_restore_regs(priv);
+
 	return 0;
 }
 
-- 
2.42.0

Re: [PATCH] i2c: i801: add helper i801_restore_regs

[Jean Delvare]

On Mon, 18 Sep 2023 13:57:01 +0200, Heiner Kallweit wrote:
> In few places relevant registers are reset to their initial value on
> driver load. Factor this out to new helper i801_restore_regs to avoid
> code duplication.
> Even though no actual problems are known, this patch may contribute
> to avoiding potential issues by:
> - restoring register values also in the error path of i2c_add_adapter
> - making restoring registers the last step (especially in i801_remove)
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> This patch is a reworked version of a part of previously discussed patch
> "i2c: i801: fix cleanup code in remove() and error path of probe()".
> ---
>  drivers/i2c/busses/i2c-i801.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 9bd712eaf..811541797 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1629,6 +1629,12 @@ static void i801_setup_hstcfg(struct i801_priv *priv)
>  	pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hstcfg);
>  }
>  
> +static void i801_restore_regs(struct i801_priv *priv)
> +{
> +	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
> +	pci_write_config_byte(priv->pci_dev, SMBHSTCFG, priv->original_hstcfg);
> +}
> +
>  static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  {
>  	int err, i;
> @@ -1755,6 +1761,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	if (err) {
>  		platform_device_unregister(priv->tco_pdev);
>  		i801_acpi_remove(priv);
> +		i801_restore_regs(priv);
>  		return err;
>  	}
>  
> @@ -1779,12 +1786,10 @@ static void i801_remove(struct pci_dev *dev)
>  {
>  	struct i801_priv *priv = pci_get_drvdata(dev);
>  
> -	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
>  	i801_disable_host_notify(priv);
>  	i801_del_mux(priv);
>  	i2c_del_adapter(&priv->adapter);
>  	i801_acpi_remove(priv);
> -	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
>  
>  	platform_device_unregister(priv->tco_pdev);
>  
> @@ -1792,6 +1797,8 @@ static void i801_remove(struct pci_dev *dev)
>  	if (!priv->acpi_reserved)
>  		pm_runtime_get_noresume(&dev->dev);
>  
> +	i801_restore_regs(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)
> @@ -1802,18 +1809,17 @@ static void i801_shutdown(struct pci_dev *dev)
>  {
>  	struct i801_priv *priv = pci_get_drvdata(dev);
>  
> -	/* Restore config registers to avoid hard hang on some systems */
> -	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
>  	i801_disable_host_notify(priv);
> -	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
> +	/* Restore config registers to avoid hard hang on some systems */
> +	i801_restore_regs(priv);
>  }
>  
>  static int i801_suspend(struct device *dev)
>  {
>  	struct i801_priv *priv = dev_get_drvdata(dev);
>  
> -	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
> -	pci_write_config_byte(priv->pci_dev, SMBHSTCFG, priv->original_hstcfg);
> +	i801_restore_regs(priv);
> +
>  	return 0;
>  }
>  

Reviewed-by: Jean Delvare <jdelvare@suse.de>

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c: i801: add helper i801_restore_regs

[Andi Shyti]

Hi Heiner,

On Mon, Sep 18, 2023 at 01:57:01PM +0200, Heiner Kallweit wrote:
> In few places relevant registers are reset to their initial value on
> driver load. Factor this out to new helper i801_restore_regs to avoid
> code duplication.
> Even though no actual problems are known, this patch may contribute
> to avoiding potential issues by:
> - restoring register values also in the error path of i2c_add_adapter
> - making restoring registers the last step (especially in i801_remove)
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Acked-by: Andi Shyti <andi.shyti@kernel.org> 

Thanks,
Andi

Re: [PATCH] i2c: i801: add helper i801_restore_regs

[Wolfram Sang]

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

On Mon, Sep 18, 2023 at 01:57:01PM +0200, Heiner Kallweit wrote:
> In few places relevant registers are reset to their initial value on
> driver load. Factor this out to new helper i801_restore_regs to avoid
> code duplication.
> Even though no actual problems are known, this patch may contribute
> to avoiding potential issues by:
> - restoring register values also in the error path of i2c_add_adapter
> - making restoring registers the last step (especially in i801_remove)
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]