[PATCH v2 0/9] i2c: i801: Series with improvements

[PATCH v2 0/9] i2c: i801: Series with improvements

[Heiner Kallweit]

This series includes a number of improvements to the i801 driver.

v2:
removed first patch from v1 series
patch 2
  - include linux/mutex.h
patch 4
  - avoid assigning potentially dangling pointer to *return_value
patch 8
  - move definition of struct i2c_board_info info to inner loop
  - add missing curly braces to outer for loop

Heiner Kallweit (9):
  i2c: i801: Improve disabling runtime pm
  i2c: i801: make p2sb_spinlock a mutex
  i2c: i801: Remove not needed debug message
  i2c: i801: Improve is_dell_system_with_lis3lv02d
  i2c: i801: Remove not needed check for PCI_COMMAND_INTX_DISABLE
  i2c: i801: Improve i801_acpi_probe/remove functions
  i2c: i801: Improve i801_add_mux
  i2c: i801: Improve register_dell_lis3lv02d_i2c_device
  i2c: i801: Improve handling platform data for tco device

 drivers/i2c/busses/i2c-i801.c | 138 ++++++++++++----------------------
 1 file changed, 48 insertions(+), 90 deletions(-)

-- 
2.32.0

[PATCH v2 1/9] i2c: i801: Improve disabling runtime pm

[Heiner Kallweit]

Setting the autosuspend delay to a negative value disables runtime pm in
a little bit smarter way, because we need no cleanup when removing the
driver. Note that this is safe when reloading the driver, because the
call to pm_runtime_set_autosuspend_delay() in probe() will reverse the
effect. See update_autosuspend() for details.

Reviewed-by: Jean Delvare <jdelvare@suse.de>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 92ec291c0..ef6dbb531 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1628,7 +1628,7 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits,
 		 * BIOS is accessing the host controller so prevent it from
 		 * suspending automatically from now on.
 		 */
-		pm_runtime_get_sync(&pdev->dev);
+		pm_runtime_set_autosuspend_delay(&pdev->dev, -1);
 	}
 
 	if ((function & ACPI_IO_MASK) == ACPI_READ)
@@ -1668,11 +1668,6 @@ static void i801_acpi_remove(struct i801_priv *priv)
 
 	acpi_remove_address_space_handler(adev->handle,
 		ACPI_ADR_SPACE_SYSTEM_IO, i801_acpi_io_handler);
-
-	mutex_lock(&priv->acpi_lock);
-	if (priv->acpi_reserved)
-		pm_runtime_put(&priv->pci_dev->dev);
-	mutex_unlock(&priv->acpi_lock);
 }
 #else
 static inline int i801_acpi_probe(struct i801_priv *priv) { return 0; }
-- 
2.32.0

[PATCH v2 2/9] i2c: i801: make p2sb_spinlock a mutex

[Heiner Kallweit]

p2sb_spinlock is used in i801_add_tco_spt() only, and in process context
only. Therefore a mutex is sufficient, and we can make the definition
local to i801_add_tco_spt().

Reviewed-by: Jean Delvare <jdelvare@suse.de>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index ef6dbb531..12e0c2ac3 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -110,6 +110,7 @@
 #include <linux/platform_device.h>
 #include <linux/platform_data/itco_wdt.h>
 #include <linux/pm_runtime.h>
+#include <linux/mutex.h>
 
 #if IS_ENABLED(CONFIG_I2C_MUX_GPIO) && defined CONFIG_DMI
 #include <linux/gpio/machine.h>
@@ -1492,12 +1493,11 @@ static const struct itco_wdt_platform_data spt_tco_platform_data = {
 	.version = 4,
 };
 
-static DEFINE_SPINLOCK(p2sb_spinlock);
-
 static struct platform_device *
 i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
 		 struct resource *tco_res)
 {
+	static DEFINE_MUTEX(p2sb_mutex);
 	struct resource *res;
 	unsigned int devfn;
 	u64 base64_addr;
@@ -1510,7 +1510,7 @@ i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
 	 * enumerated by the PCI subsystem, so we need to unhide/hide it
 	 * to lookup the P2SB BAR.
 	 */
-	spin_lock(&p2sb_spinlock);
+	mutex_lock(&p2sb_mutex);
 
 	devfn = PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 1);
 
@@ -1528,7 +1528,7 @@ i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
 	/* Hide the P2SB device, if it was hidden before */
 	if (hidden)
 		pci_bus_write_config_byte(pci_dev->bus, devfn, 0xe1, hidden);
-	spin_unlock(&p2sb_spinlock);
+	mutex_unlock(&p2sb_mutex);
 
 	res = &tco_res[1];
 	if (pci_dev->device == PCI_DEVICE_ID_INTEL_DNV_SMBUS)
-- 
2.32.0

[PATCH v2 3/9] i2c: i801: Remove not needed debug message

[Heiner Kallweit]

If a user is interested in such details he can enable smbus tracing.

Reviewed-by: Jean Delvare <jdelvare@suse.de>
Tested-by: Jean Delvare <jdelvare@suse.de>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 12e0c2ac3..89ae78ef1 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -559,10 +559,6 @@ static void i801_isr_byte_done(struct i801_priv *priv)
 					priv->len);
 				/* FIXME: Recover */
 				priv->len = I2C_SMBUS_BLOCK_MAX;
-			} else {
-				dev_dbg(&priv->pci_dev->dev,
-					"SMBus block read size is %d\n",
-					priv->len);
 			}
 			priv->data[-1] = priv->len;
 		}
-- 
2.32.0

[PATCH v2 4/9] i2c: i801: Improve is_dell_system_with_lis3lv02d

[Heiner Kallweit]

Replace the ugly cast of the return_value pointer with proper usage.
In addition use dmi_match() instead of open-coding it.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
  - avoid assigning potentially dangling pointer to *return_value
---
 drivers/i2c/busses/i2c-i801.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 89ae78ef1..f56060fcf 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1192,7 +1192,7 @@ static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle,
 
 	kfree(info);
 
-	*((bool *)return_value) = true;
+	*return_value = NULL;
 	return AE_CTRL_TERMINATE;
 
 smo88xx_not_found:
@@ -1202,11 +1202,9 @@ static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle,
 
 static bool is_dell_system_with_lis3lv02d(void)
 {
-	bool found;
-	const char *vendor;
+	void *err = ERR_PTR(-ENOENT);
 
-	vendor = dmi_get_system_info(DMI_SYS_VENDOR);
-	if (!vendor || strcmp(vendor, "Dell Inc."))
+	if (!dmi_match(DMI_SYS_VENDOR, "Dell Inc."))
 		return false;
 
 	/*
@@ -1217,11 +1215,9 @@ static bool is_dell_system_with_lis3lv02d(void)
 	 * accelerometer but unfortunately ACPI does not provide any other
 	 * information (like I2C address).
 	 */
-	found = false;
-	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
-			 (void **)&found);
+	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &err);
 
-	return found;
+	return !IS_ERR(err);
 }
 
 /*
-- 
2.32.0

[PATCH v2 5/9] i2c: i801: Remove not needed check for PCI_COMMAND_INTX_DISABLE

[Heiner Kallweit]

do_pci_enable_device() takes care that PCI_COMMAND_INTX_DISABLE
is cleared if a legacy interrupt is used.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index f56060fcf..7fa06b85f 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1827,19 +1827,12 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		priv->features &= ~FEATURE_IRQ;
 
 	if (priv->features & FEATURE_IRQ) {
-		u16 pcictl, pcists;
+		u16 pcists;
 
 		/* Complain if an interrupt is already pending */
 		pci_read_config_word(priv->pci_dev, PCI_STATUS, &pcists);
 		if (pcists & PCI_STATUS_INTERRUPT)
 			dev_warn(&dev->dev, "An interrupt is pending!\n");
-
-		/* Check if interrupts have been disabled */
-		pci_read_config_word(priv->pci_dev, PCI_COMMAND, &pcictl);
-		if (pcictl & PCI_COMMAND_INTX_DISABLE) {
-			dev_info(&dev->dev, "Interrupts are disabled\n");
-			priv->features &= ~FEATURE_IRQ;
-		}
 	}
 
 	if (priv->features & FEATURE_IRQ) {
-- 
2.32.0

[PATCH v2 6/9] i2c: i801: Improve i801_acpi_probe/remove functions

[Heiner Kallweit]

By using ACPI_HANDLE() the handler argument can be retrieved directly.
Both address space handler functions check the handler argument and
return an error if it's NULL. This allows to further simplify the code.

Reviewed-by: Jean Delvare <jdelvare@suse.de>
Tested-by: Jean Delvare <jdelvare@suse.de>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 7fa06b85f..bd1db0f0a 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1635,31 +1635,22 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits,
 
 static int i801_acpi_probe(struct i801_priv *priv)
 {
-	struct acpi_device *adev;
+	acpi_handle ah = ACPI_HANDLE(&priv->pci_dev->dev);
 	acpi_status status;
 
-	adev = ACPI_COMPANION(&priv->pci_dev->dev);
-	if (adev) {
-		status = acpi_install_address_space_handler(adev->handle,
-				ACPI_ADR_SPACE_SYSTEM_IO, i801_acpi_io_handler,
-				NULL, priv);
-		if (ACPI_SUCCESS(status))
-			return 0;
-	}
+	status = acpi_install_address_space_handler(ah, ACPI_ADR_SPACE_SYSTEM_IO,
+						    i801_acpi_io_handler, NULL, priv);
+	if (ACPI_SUCCESS(status))
+		return 0;
 
 	return acpi_check_resource_conflict(&priv->pci_dev->resource[SMBBAR]);
 }
 
 static void i801_acpi_remove(struct i801_priv *priv)
 {
-	struct acpi_device *adev;
-
-	adev = ACPI_COMPANION(&priv->pci_dev->dev);
-	if (!adev)
-		return;
+	acpi_handle ah = ACPI_HANDLE(&priv->pci_dev->dev);
 
-	acpi_remove_address_space_handler(adev->handle,
-		ACPI_ADR_SPACE_SYSTEM_IO, i801_acpi_io_handler);
+	acpi_remove_address_space_handler(ah, ACPI_ADR_SPACE_SYSTEM_IO, i801_acpi_io_handler);
 }
 #else
 static inline int i801_acpi_probe(struct i801_priv *priv) { return 0; }
-- 
2.32.0

[PATCH v2 7/9] i2c: i801: Improve i801_add_mux

[Heiner Kallweit]

The return value of i801_add_mux() isn't used, so let's change it to void.
In addition remove the not needed cast to struct gpiod_lookup.
GPIO_LOOKUP() uses GPIO_LOOKUP_IDX() that includes this cast.

Reviewed-by: Jean Delvare <jdelvare@suse.de>
Tested-by: Jean Delvare <jdelvare@suse.de>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index bd1db0f0a..6e9aca81b 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1391,7 +1391,7 @@ static const struct dmi_system_id mux_dmi_table[] = {
 };
 
 /* Setup multiplexing if needed */
-static int i801_add_mux(struct i801_priv *priv)
+static void i801_add_mux(struct i801_priv *priv)
 {
 	struct device *dev = &priv->adapter.dev;
 	const struct i801_mux_config *mux_config;
@@ -1400,7 +1400,7 @@ static int i801_add_mux(struct i801_priv *priv)
 	int i;
 
 	if (!priv->mux_drvdata)
-		return 0;
+		return;
 	mux_config = priv->mux_drvdata;
 
 	/* Prepare the platform data */
@@ -1416,13 +1416,11 @@ static int i801_add_mux(struct i801_priv *priv)
 			      struct_size(lookup, table, mux_config->n_gpios + 1),
 			      GFP_KERNEL);
 	if (!lookup)
-		return -ENOMEM;
+		return;
 	lookup->dev_id = "i2c-mux-gpio";
-	for (i = 0; i < mux_config->n_gpios; i++) {
-		lookup->table[i] = (struct gpiod_lookup)
-			GPIO_LOOKUP(mux_config->gpio_chip,
-				    mux_config->gpios[i], "mux", 0);
-	}
+	for (i = 0; i < mux_config->n_gpios; i++)
+		lookup->table[i] = GPIO_LOOKUP(mux_config->gpio_chip,
+					       mux_config->gpios[i], "mux", 0);
 	gpiod_add_lookup_table(lookup);
 	priv->lookup = lookup;
 
@@ -1440,8 +1438,6 @@ static int i801_add_mux(struct i801_priv *priv)
 		gpiod_remove_lookup_table(lookup);
 		dev_err(dev, "Failed to register i2c-mux-gpio device\n");
 	}
-
-	return PTR_ERR_OR_ZERO(priv->mux_pdev);
 }
 
 static void i801_del_mux(struct i801_priv *priv)
@@ -1471,7 +1467,7 @@ static unsigned int i801_get_adapter_class(struct i801_priv *priv)
 	return class;
 }
 #else
-static inline int i801_add_mux(struct i801_priv *priv) { return 0; }
+static inline void i801_add_mux(struct i801_priv *priv) { }
 static inline void i801_del_mux(struct i801_priv *priv) { }
 
 static inline unsigned int i801_get_adapter_class(struct i801_priv *priv)
-- 
2.32.0

[PATCH v2 8/9] i2c: i801: Improve register_dell_lis3lv02d_i2c_device

[Heiner Kallweit]

- Use an initializer for struct i2c_board_info info
- Use dmi_match()
- Simplify loop logic

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
  - move definition of struct i2c_board_info info to inner loop
  - add missing curly braces to outer for loop
---
 drivers/i2c/busses/i2c-i801.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 6e9aca81b..88745e3bc 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1247,28 +1247,22 @@ static const struct {
 
 static void register_dell_lis3lv02d_i2c_device(struct i801_priv *priv)
 {
-	struct i2c_board_info info;
-	const char *dmi_product_name;
 	int i;
 
-	dmi_product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
 	for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i) {
-		if (strcmp(dmi_product_name,
-			   dell_lis3lv02d_devices[i].dmi_product_name) == 0)
-			break;
-	}
-
-	if (i == ARRAY_SIZE(dell_lis3lv02d_devices)) {
-		dev_warn(&priv->pci_dev->dev,
-			 "Accelerometer lis3lv02d is present on SMBus but its"
-			 " address is unknown, skipping registration\n");
-		return;
+		if (dmi_match(DMI_PRODUCT_NAME, dell_lis3lv02d_devices[i].dmi_product_name)) {
+			struct i2c_board_info info = {
+				.type = "lis3lv02d",
+				.addr = dell_lis3lv02d_devices[i].i2c_addr,
+			};
+
+			i2c_new_client_device(&priv->adapter, &info);
+			return;
+		}
 	}
 
-	memset(&info, 0, sizeof(struct i2c_board_info));
-	info.addr = dell_lis3lv02d_devices[i].i2c_addr;
-	strlcpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
-	i2c_new_client_device(&priv->adapter, &info);
+	pci_warn(priv->pci_dev,
+		 "Accelerometer lis3lv02d is present on SMBus but its address is unknown, skipping registration\n");
 }
 
 /* Register optional slaves */
-- 
2.32.0

[PATCH v2 9/9] i2c: i801: Improve handling platform data for tco device

[Heiner Kallweit]

The platform data structures are used in the respective i801_add_tco
functions only. Therefore we can make the definitions local to these
functions.

Reviewed-by: Jean Delvare <jdelvare@suse.de>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 88745e3bc..242bdd2ae 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1470,15 +1470,14 @@ static inline unsigned int i801_get_adapter_class(struct i801_priv *priv)
 }
 #endif
 
-static const struct itco_wdt_platform_data spt_tco_platform_data = {
-	.name = "Intel PCH",
-	.version = 4,
-};
-
 static struct platform_device *
 i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
 		 struct resource *tco_res)
 {
+	static const struct itco_wdt_platform_data pldata = {
+		.name = "Intel PCH",
+		.version = 4,
+	};
 	static DEFINE_MUTEX(p2sb_mutex);
 	struct resource *res;
 	unsigned int devfn;
@@ -1522,22 +1521,20 @@ i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
 	res->flags = IORESOURCE_MEM;
 
 	return platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
-					tco_res, 2, &spt_tco_platform_data,
-					sizeof(spt_tco_platform_data));
+					tco_res, 2, &pldata, sizeof(pldata));
 }
 
-static const struct itco_wdt_platform_data cnl_tco_platform_data = {
-	.name = "Intel PCH",
-	.version = 6,
-};
-
 static struct platform_device *
 i801_add_tco_cnl(struct i801_priv *priv, struct pci_dev *pci_dev,
 		 struct resource *tco_res)
 {
-	return platform_device_register_resndata(&pci_dev->dev,
-			"iTCO_wdt", -1, tco_res, 1, &cnl_tco_platform_data,
-			sizeof(cnl_tco_platform_data));
+	static const struct itco_wdt_platform_data pldata = {
+		.name = "Intel PCH",
+		.version = 6,
+	};
+
+	return platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
+						 tco_res, 1, &pldata, sizeof(pldata));
 }
 
 static void i801_add_tco(struct i801_priv *priv)
-- 
2.32.0

Re: [PATCH v2 1/9] i2c: i801: Improve disabling runtime pm

[Wolfram Sang]

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

On Fri, Aug 06, 2021 at 11:12:18PM +0200, Heiner Kallweit wrote:
> Setting the autosuspend delay to a negative value disables runtime pm in
> a little bit smarter way, because we need no cleanup when removing the
> driver. Note that this is safe when reloading the driver, because the
> call to pm_runtime_set_autosuspend_delay() in probe() will reverse the
> effect. See update_autosuspend() for details.
> 
> Reviewed-by: Jean Delvare <jdelvare@suse.de>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Applied to for-next, thanks!


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

Re: [PATCH v2 2/9] i2c: i801: make p2sb_spinlock a mutex

[Wolfram Sang]

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

On Fri, Aug 06, 2021 at 11:13:29PM +0200, Heiner Kallweit wrote:
> p2sb_spinlock is used in i801_add_tco_spt() only, and in process context
> only. Therefore a mutex is sufficient, and we can make the definition
> local to i801_add_tco_spt().
> 
> Reviewed-by: Jean Delvare <jdelvare@suse.de>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Applied to for-next, thanks!

>  #include <linux/platform_device.h>
>  #include <linux/platform_data/itco_wdt.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/mutex.h>

Can you also sort the includes while cleaning up the driver? Then, it is
more obvious that mutex.h was already needed for acpi_lock. That would
be great!


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

Re: [PATCH v2 3/9] i2c: i801: Remove not needed debug message

[Wolfram Sang]

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

On Fri, Aug 06, 2021 at 11:14:08PM +0200, Heiner Kallweit wrote:
> If a user is interested in such details he can enable smbus tracing.
> 
> Reviewed-by: Jean Delvare <jdelvare@suse.de>
> Tested-by: Jean Delvare <jdelvare@suse.de>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Applied to for-next, thanks! Waiting for further reviews for the
following patches now.


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

Re: [PATCH v2 1/9] i2c: i801: Improve disabling runtime pm

[Andy Shevchenko]

On Fri, Aug 06, 2021 at 11:12:18PM +0200, Heiner Kallweit wrote:
> Setting the autosuspend delay to a negative value disables runtime pm in
> a little bit smarter way, because we need no cleanup when removing the
> driver. Note that this is safe when reloading the driver, because the
> call to pm_runtime_set_autosuspend_delay() in probe() will reverse the
> effect. See update_autosuspend() for details.

...

>  		 * BIOS is accessing the host controller so prevent it from
>  		 * suspending automatically from now on.
>  		 */
> -		pm_runtime_get_sync(&pdev->dev);
> +		pm_runtime_set_autosuspend_delay(&pdev->dev, -1);

I dunno if it's being discussed, but with this you effectively allow user to
override the setting. It may screw things up AFAIU the comment above.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/9] i2c: i801: make p2sb_spinlock a mutex

[Andy Shevchenko]

On Fri, Aug 06, 2021 at 11:13:29PM +0200, Heiner Kallweit wrote:
> p2sb_spinlock is used in i801_add_tco_spt() only, and in process context
> only. Therefore a mutex is sufficient, and we can make the definition
> local to i801_add_tco_spt().

The problem with either AFAICT is that it should actually hold PCI rescan lock.
See the discussion with Message-ID
20210308122020.57071-1-andriy.shevchenko@linux.intel.com for the details.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/9] i2c: i801: Improve is_dell_system_with_lis3lv02d

[Andy Shevchenko]

On Fri, Aug 06, 2021 at 11:15:15PM +0200, Heiner Kallweit wrote:
> Replace the ugly cast of the return_value pointer with proper usage.
> In addition use dmi_match() instead of open-coding it.

...

> -	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
> -			 (void **)&found);
> +	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &err);
>  
> -	return found;
> +	return !IS_ERR(err);

Shouldn't you also check the status of acpi_get_device()?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/9] i2c: i801: Remove not needed check for PCI_COMMAND_INTX_DISABLE

[Andy Shevchenko]

On Fri, Aug 06, 2021 at 11:15:51PM +0200, Heiner Kallweit wrote:
> do_pci_enable_device() takes care that PCI_COMMAND_INTX_DISABLE
> is cleared if a legacy interrupt is used.

Makes sense, but needs to be tested.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index f56060fcf..7fa06b85f 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1827,19 +1827,12 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  		priv->features &= ~FEATURE_IRQ;
>  
>  	if (priv->features & FEATURE_IRQ) {
> -		u16 pcictl, pcists;
> +		u16 pcists;
>  
>  		/* Complain if an interrupt is already pending */
>  		pci_read_config_word(priv->pci_dev, PCI_STATUS, &pcists);
>  		if (pcists & PCI_STATUS_INTERRUPT)
>  			dev_warn(&dev->dev, "An interrupt is pending!\n");
> -
> -		/* Check if interrupts have been disabled */
> -		pci_read_config_word(priv->pci_dev, PCI_COMMAND, &pcictl);
> -		if (pcictl & PCI_COMMAND_INTX_DISABLE) {
> -			dev_info(&dev->dev, "Interrupts are disabled\n");
> -			priv->features &= ~FEATURE_IRQ;
> -		}
>  	}
>  
>  	if (priv->features & FEATURE_IRQ) {
> -- 
> 2.32.0
> 
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 8/9] i2c: i801: Improve register_dell_lis3lv02d_i2c_device

[Andy Shevchenko]

On Fri, Aug 06, 2021 at 11:18:05PM +0200, Heiner Kallweit wrote:
> - Use an initializer for struct i2c_board_info info
> - Use dmi_match()
> - Simplify loop logic

I'm wondering if changing this to a DMI match table will give better result.

Something like
(Sorry I forgot APIs, but plenty of examples are under PDx86: drivers/platform/x86):

struct dmi_..._id *id;

id = dmi_..._match();
if (!id) {
	pci_warn();
	return;
}

i2c_new_client_device(...);


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 9/9] i2c: i801: Improve handling platform data for tco device

[Andy Shevchenko]

On Fri, Aug 06, 2021 at 11:18:40PM +0200, Heiner Kallweit wrote:
> The platform data structures are used in the respective i801_add_tco
> functions only. Therefore we can make the definitions local to these
> functions.

Side note: usually we refer to the functions like "i801_add_tco()" (w/o
quotes).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/9] i2c: i801: Improve disabling runtime pm

[Heiner Kallweit]

On 11.08.2021 17:41, Andy Shevchenko wrote:
> On Fri, Aug 06, 2021 at 11:12:18PM +0200, Heiner Kallweit wrote:
>> Setting the autosuspend delay to a negative value disables runtime pm in
>> a little bit smarter way, because we need no cleanup when removing the
>> driver. Note that this is safe when reloading the driver, because the
>> call to pm_runtime_set_autosuspend_delay() in probe() will reverse the
>> effect. See update_autosuspend() for details.
> 
> ...
> 
>>  		 * BIOS is accessing the host controller so prevent it from
>>  		 * suspending automatically from now on.
>>  		 */
>> -		pm_runtime_get_sync(&pdev->dev);
>> +		pm_runtime_set_autosuspend_delay(&pdev->dev, -1);
> 
> I dunno if it's being discussed, but with this you effectively allow user to
> override the setting. It may screw things up AFAIU the comment above.
> 
No, this hasn't been discussed. At least not now. Thanks for the hint.
This attribute is writable for the root user, so we could argue that
the root user has several options to break the system anyway.

Re: [PATCH v2 2/9] i2c: i801: make p2sb_spinlock a mutex

[Heiner Kallweit]

On 11.08.2021 17:43, Andy Shevchenko wrote:
> On Fri, Aug 06, 2021 at 11:13:29PM +0200, Heiner Kallweit wrote:
>> p2sb_spinlock is used in i801_add_tco_spt() only, and in process context
>> only. Therefore a mutex is sufficient, and we can make the definition
>> local to i801_add_tco_spt().
> 
> The problem with either AFAICT is that it should actually hold PCI rescan lock.
> See the discussion with Message-ID
> 20210308122020.57071-1-andriy.shevchenko@linux.intel.com for the details.
> 
Thanks for the link. I see that you use pci_lock_rescan_remove() but at a first
glance didn't see this being discussed. Maybe because it's obvious ..

i801 was discussed here:
https://lore.kernel.org/linux-i2c/20210310155145.513a7165@endymion/
However discussion seems to have ended w/o result. What's the status of your
p2sb series? Backgroud of the question is: Does it make sense to wait for
your series to be applied, to make use of your new ps2b library functions?
Or change the mutex to the rescan mutex for the time being?

Re: [PATCH v2 4/9] i2c: i801: Improve is_dell_system_with_lis3lv02d

[Heiner Kallweit]

On 11.08.2021 17:45, Andy Shevchenko wrote:
> On Fri, Aug 06, 2021 at 11:15:15PM +0200, Heiner Kallweit wrote:
>> Replace the ugly cast of the return_value pointer with proper usage.
>> In addition use dmi_match() instead of open-coding it.
> 
> ...
> 
>> -	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
>> -			 (void **)&found);
>> +	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &err);
>>  
>> -	return found;
>> +	return !IS_ERR(err);
> 
> Shouldn't you also check the status of acpi_get_device()?
> 
This shouldn't be needed because err isn't touched if function fails.

Re: [PATCH v2 8/9] i2c: i801: Improve register_dell_lis3lv02d_i2c_device

[Heiner Kallweit]

On 11.08.2021 17:52, Andy Shevchenko wrote:
> On Fri, Aug 06, 2021 at 11:18:05PM +0200, Heiner Kallweit wrote:
>> - Use an initializer for struct i2c_board_info info
>> - Use dmi_match()
>> - Simplify loop logic
> 
> I'm wondering if changing this to a DMI match table will give better result.
> 
> Something like
> (Sorry I forgot APIs, but plenty of examples are under PDx86: drivers/platform/x86):
> 
> struct dmi_..._id *id;
> 
> id = dmi_..._match();
> if (!id) {
> 	pci_warn();
> 	return;
> }
> 
> i2c_new_client_device(...);
> 
> 

We could do something like the following. Whether it's better may be a
question of personal taste. I have no strong opinion here and would leave
it to Jean.

const struct dmi_system_id lis3_id_table[] = {
        {
                .driver_data = (void *)0x29,
                .matches = {
                        DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5550"),
                },
        },
	...

dmi_system_id *id = dmi_first_match(lis3_id_table);
if (id)
	i2c_new_client_device(..., (unsigned int)id->driver_data;
else
	lament()

Re: [PATCH v2 1/9] i2c: i801: Improve disabling runtime pm

[Andy Shevchenko]

On Wed, Aug 11, 2021 at 10:03:12PM +0200, Heiner Kallweit wrote:
> On 11.08.2021 17:41, Andy Shevchenko wrote:
> > On Fri, Aug 06, 2021 at 11:12:18PM +0200, Heiner Kallweit wrote:
> >> Setting the autosuspend delay to a negative value disables runtime pm in
> >> a little bit smarter way, because we need no cleanup when removing the
> >> driver. Note that this is safe when reloading the driver, because the
> >> call to pm_runtime_set_autosuspend_delay() in probe() will reverse the
> >> effect. See update_autosuspend() for details.
> > 
> > ...
> > 
> >>  		 * BIOS is accessing the host controller so prevent it from
> >>  		 * suspending automatically from now on.
> >>  		 */
> >> -		pm_runtime_get_sync(&pdev->dev);
> >> +		pm_runtime_set_autosuspend_delay(&pdev->dev, -1);
> > 
> > I dunno if it's being discussed, but with this you effectively allow user to
> > override the setting. It may screw things up AFAIU the comment above.
> > 
> No, this hasn't been discussed. At least not now. Thanks for the hint.
> This attribute is writable for the root user, so we could argue that
> the root user has several options to break the system anyway.

But it will mean the side effect on this driver and typical (root-run) system
application (systemd like?) should care now the knowledge about this
side-effect. I do not think it is desired behaviour. But I'm not a maintainer
and I commented here just to make everybody understand the consequences of the
change.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/9] i2c: i801: make p2sb_spinlock a mutex

[Andy Shevchenko]

On Wed, Aug 11, 2021 at 10:27:26PM +0200, Heiner Kallweit wrote:
> On 11.08.2021 17:43, Andy Shevchenko wrote:
> > On Fri, Aug 06, 2021 at 11:13:29PM +0200, Heiner Kallweit wrote:
> >> p2sb_spinlock is used in i801_add_tco_spt() only, and in process context
> >> only. Therefore a mutex is sufficient, and we can make the definition
> >> local to i801_add_tco_spt().
> > 
> > The problem with either AFAICT is that it should actually hold PCI rescan lock.
> > See the discussion with Message-ID
> > 20210308122020.57071-1-andriy.shevchenko@linux.intel.com for the details.
> > 
> Thanks for the link. I see that you use pci_lock_rescan_remove() but at a first
> glance didn't see this being discussed. Maybe because it's obvious ..
> 
> i801 was discussed here:
> https://lore.kernel.org/linux-i2c/20210310155145.513a7165@endymion/
> However discussion seems to have ended w/o result. What's the status of your
> p2sb series? Backgroud of the question is: Does it make sense to wait for
> your series to be applied, to make use of your new ps2b library functions?
> Or change the mutex to the rescan mutex for the time being?

The problem with P2SB PCI device is that it's used as a holder for the firmware
programmed value of the BAR which mustn't be relocated. If PCI runs rescan
concurrently with this piece of code (still a probability higher than 0) then
we will be in bad situation.

So, yes, rescan lock is a minimum what has to be done.

In regard to the series the idea is to unhide the P2SB in early PCI quirk and
mark its resources unrelocatable. But I haven't time to research that. It's
postponed currently due to lack of time on my side because higher priority
tasks ongoing.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/9] i2c: i801: Improve is_dell_system_with_lis3lv02d

[Andy Shevchenko]

On Wed, Aug 11, 2021 at 10:28:25PM +0200, Heiner Kallweit wrote:
> On 11.08.2021 17:45, Andy Shevchenko wrote:
> > On Fri, Aug 06, 2021 at 11:15:15PM +0200, Heiner Kallweit wrote:
> >> Replace the ugly cast of the return_value pointer with proper usage.
> >> In addition use dmi_match() instead of open-coding it.
> > 
> > ...
> > 
> >> -	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
> >> -			 (void **)&found);
> >> +	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &err);
> >>  
> >> -	return found;
> >> +	return !IS_ERR(err);
> > 
> > Shouldn't you also check the status of acpi_get_device()?
> > 
> This shouldn't be needed because err isn't touched if function fails.

For the sake of clearness of the code I would do it. But in any case what
really hurt my eye is the last line here. To me sounds like

	if (IS_ERR(err))
		return false;
	return true;

is much better to read (and I bet the compiler will generate the very same
code for it).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 8/9] i2c: i801: Improve register_dell_lis3lv02d_i2c_device

[Andy Shevchenko]

On Wed, Aug 11, 2021 at 11:05:06PM +0200, Heiner Kallweit wrote:
> On 11.08.2021 17:52, Andy Shevchenko wrote:
> > On Fri, Aug 06, 2021 at 11:18:05PM +0200, Heiner Kallweit wrote:
> >> - Use an initializer for struct i2c_board_info info
> >> - Use dmi_match()
> >> - Simplify loop logic
> > 
> > I'm wondering if changing this to a DMI match table will give better result.
> > 
> > Something like
> > (Sorry I forgot APIs, but plenty of examples are under PDx86: drivers/platform/x86):
> > 
> > struct dmi_..._id *id;
> > 
> > id = dmi_..._match();
> > if (!id) {
> > 	pci_warn();
> > 	return;
> > }
> > 
> > i2c_new_client_device(...);
> > 
> We could do something like the following. Whether it's better may be a
> question of personal taste. I have no strong opinion here and would leave
> it to Jean.
> 
> const struct dmi_system_id lis3_id_table[] = {
>         {
>                 .driver_data = (void *)0x29,
>                 .matches = {
>                         DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5550"),
>                 },
>         },
> 	...
> 
> dmi_system_id *id = dmi_first_match(lis3_id_table);
> if (id)
> 	i2c_new_client_device(..., (unsigned int)id->driver_data;
> else
> 	lament()

Yep, my point here that this has less indentation of the code, no unneeded
for-loop (which will be inside DMI APIs, etc).

But yes, I agree that this is rather matter of taste.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/9] i2c: i801: Improve disabling runtime pm

[Wolfram Sang]

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


> > > I dunno if it's being discussed, but with this you effectively allow user to
> > > override the setting. It may screw things up AFAIU the comment above.
> > > 
> > No, this hasn't been discussed. At least not now. Thanks for the hint.
> > This attribute is writable for the root user, so we could argue that
> > the root user has several options to break the system anyway.
> 
> But it will mean the side effect on this driver and typical (root-run) system
> application (systemd like?) should care now the knowledge about this
> side-effect. I do not think it is desired behaviour. But I'm not a maintainer
> and I commented here just to make everybody understand the consequences of the
> change.

Jean, are you still fine with this patch then?


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

Re: [PATCH v2 4/9] i2c: i801: Improve is_dell_system_with_lis3lv02d

[Jean Delvare]

Hi Heiner,

On Fri, 06 Aug 2021 23:15:15 +0200, Heiner Kallweit wrote:
> Replace the ugly cast of the return_value pointer with proper usage.
> In addition use dmi_match() instead of open-coding it.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v2:
>   - avoid assigning potentially dangling pointer to *return_value
> ---
>  drivers/i2c/busses/i2c-i801.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 89ae78ef1..f56060fcf 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1192,7 +1192,7 @@ static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle,
>  
>  	kfree(info);
>  
> -	*((bool *)return_value) = true;
> +	*return_value = NULL;
>  	return AE_CTRL_TERMINATE;
>  
>  smo88xx_not_found:
> @@ -1202,11 +1202,9 @@ static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle,
>  
>  static bool is_dell_system_with_lis3lv02d(void)
>  {
> -	bool found;
> -	const char *vendor;
> +	void *err = ERR_PTR(-ENOENT);
>  
> -	vendor = dmi_get_system_info(DMI_SYS_VENDOR);
> -	if (!vendor || strcmp(vendor, "Dell Inc."))
> +	if (!dmi_match(DMI_SYS_VENDOR, "Dell Inc."))
>  		return false;
>  
>  	/*
> @@ -1217,11 +1215,9 @@ static bool is_dell_system_with_lis3lv02d(void)
>  	 * accelerometer but unfortunately ACPI does not provide any other
>  	 * information (like I2C address).
>  	 */
> -	found = false;
> -	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
> -			 (void **)&found);
> +	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &err);
>  
> -	return found;
> +	return !IS_ERR(err);
>  }
>  
>  /*

OK, it's a bit different from what I proposed but it still addresses the
concerns that had been raised. So fine with me.

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

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2 1/9] i2c: i801: Improve disabling runtime pm

[Jean Delvare]

Hi Wolfram,

On Tue, 17 Aug 2021 22:15:58 +0200, Wolfram Sang wrote:
> > > > I dunno if it's being discussed, but with this you effectively allow user to
> > > > override the setting. It may screw things up AFAIU the comment above.
> > >
> > > No, this hasn't been discussed. At least not now. Thanks for the hint.
> > > This attribute is writable for the root user, so we could argue that
> > > the root user has several options to break the system anyway.  

This is something we hear frequently when people don't want to address
problems in their code, but that's not enough to convince me ;-)

> > But it will mean the side effect on this driver and typical (root-run) system
> > application (systemd like?) should care now the knowledge about this
> > side-effect. I do not think it is desired behaviour. But I'm not a maintainer
> > and I commented here just to make everybody understand the consequences of the
> > change.  

Is systemd going to actually make any change to that attribute? I'm no
systemd expert, but I can't see any option in the configuration files
that would be related to autosuspend.

> Jean, are you still fine with this patch then?

My original position was that there are a few other drivers already
doing "this". It's not like we are doing something completely new and
using an API in a way it had never been used before, so it can't be
that bad.

On the other hand, after taking a closer look, I'm not fully certain
that "this" is exactly the same in all these drivers. For example, in
blk-pm.c, pm_runtime_set_autosuspend_delay() is being called with value
-1 initially, but with the idea that someone else (device driver, user)
may set a positive value later. It's not a permanent disable. The
8250_omap driver, however, seems to match the i2c-i801 driver here (I
say "seems" because honestly I'm not sure I fully understand the
comments there, but my understanding is that at least in some
situations, enabling autosuspend later would cause problems).

That being said, it starts looking like a problem for the PM subsystem
maintainers. Basically Heiner is trying to move away from an API which
requires cleaning up on driver removal. This is definitely the
direction we are collectively taking for years now (the whole devm_*
family of functions is about exactly that). So it's considered a good
thing.

If pm_runtime_set_autosuspend_delay() is not suitable for the task then
maybe we need a better API. I will admit I'm at a loss when it comes to
the many pm_runtime_* calls, I'm not going to claim I fully understand
what each of them is doing exactly. But don't we want to simply call
pm_runtime_dont_use_autosuspend() here?

If not and there's no suitable API for the task at the moment, then
better do not apply this patch, and instead ask the PM subsystem
maintainers if they would be willing to implement what we need.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2 4/9] i2c: i801: Improve is_dell_system_with_lis3lv02d

[Jean Delvare]

On Thu, 12 Aug 2021 12:55:20 +0300, Andy Shevchenko wrote:
> On Wed, Aug 11, 2021 at 10:28:25PM +0200, Heiner Kallweit wrote:
> > On 11.08.2021 17:45, Andy Shevchenko wrote:  
> > > On Fri, Aug 06, 2021 at 11:15:15PM +0200, Heiner Kallweit wrote:  
> > >> Replace the ugly cast of the return_value pointer with proper usage.
> > >> In addition use dmi_match() instead of open-coding it.  
> > > 
> > > ...
> > >   
> > >> -	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
> > >> -			 (void **)&found);
> > >> +	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &err);
> > >>  
> > >> -	return found;
> > >> +	return !IS_ERR(err);  
> > > 
> > > Shouldn't you also check the status of acpi_get_device()?
> >
> > This shouldn't be needed because err isn't touched if function fails.  
> 
> For the sake of clearness of the code I would do it.

This brings us back to how awkward the API is. Most callers don't
bother checking the return value of acpi_get_devices() because it's
useless in practice. But I agree that in theory it could return with an
error and then it would be nicer to catch that.

> (...) But in any case what
> really hurt my eye is the last line here. To me sounds like
> 
> 	if (IS_ERR(err))
> 		return false;
> 	return true;
> 
> is much better to read (and I bet the compiler will generate the very same
> code for it).

Somehow the assembly code differs, but I'm unable to see the relation
between your proposed change and the assembly code changes. That's why
I hate modern compilers. They pretend to be smart, but what they are
essentially is unstable, and this ruins any attempt at such trivial
comparisons. Sad.

Personally I don't really care, Heiner's code did not strike me as
being hard to read in the first place. I tend to avoid conditionals
when possible.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2 1/9] i2c: i801: Improve disabling runtime pm

[Andy Shevchenko]

On Thu, Aug 26, 2021 at 04:00:21PM +0200, Jean Delvare wrote:
> On Tue, 17 Aug 2021 22:15:58 +0200, Wolfram Sang wrote:

Jean, thanks for your response. My 2 cents below.

> > > > > I dunno if it's being discussed, but with this you effectively allow user to
> > > > > override the setting. It may screw things up AFAIU the comment above.
> > > >
> > > > No, this hasn't been discussed. At least not now. Thanks for the hint.
> > > > This attribute is writable for the root user, so we could argue that
> > > > the root user has several options to break the system anyway.
> 
> This is something we hear frequently when people don't want to address
> problems in their code, but that's not enough to convince me ;-)
> 
> > > But it will mean the side effect on this driver and typical (root-run) system
> > > application (systemd like?) should care now the knowledge about this
> > > side-effect. I do not think it is desired behaviour. But I'm not a maintainer
> > > and I commented here just to make everybody understand the consequences of the
> > > change.
> 
> Is systemd going to actually make any change to that attribute? I'm no
> systemd expert, but I can't see any option in the configuration files
> that would be related to autosuspend.
> 
> > Jean, are you still fine with this patch then?
> 
> My original position was that there are a few other drivers already
> doing "this". It's not like we are doing something completely new and
> using an API in a way it had never been used before, so it can't be
> that bad.
> 
> On the other hand, after taking a closer look, I'm not fully certain
> that "this" is exactly the same in all these drivers. For example, in
> blk-pm.c, pm_runtime_set_autosuspend_delay() is being called with value
> -1 initially, but with the idea that someone else (device driver, user)
> may set a positive value later. It's not a permanent disable.

Correct, it's expected that either system wide tool or user with enough
privileges may enable it.

> The 8250_omap driver,

Oh, no, please, do not use that driver as an example for runtime PM. It's
(somehow) broken there! I Cc'ed Tony in case you have more Q:s about it.
It's on a healing curve, though.

The idea behind I believe is the same, i.e. to allow user to turn it on
and off.

> however, seems to match the i2c-i801 driver here (I
> say "seems" because honestly I'm not sure I fully understand the
> comments there, but my understanding is that at least in some
> situations, enabling autosuspend later would cause problems).
> 
> That being said, it starts looking like a problem for the PM subsystem
> maintainers. Basically Heiner is trying to move away from an API which
> requires cleaning up on driver removal. This is definitely the
> direction we are collectively taking for years now (the whole devm_*
> family of functions is about exactly that). So it's considered a good
> thing.
> 
> If pm_runtime_set_autosuspend_delay() is not suitable for the task then
> maybe we need a better API. I will admit I'm at a loss when it comes to
> the many pm_runtime_* calls, I'm not going to claim I fully understand
> what each of them is doing exactly. But don't we want to simply call
> pm_runtime_dont_use_autosuspend() here?
> 
> If not and there's no suitable API for the task at the moment, then
> better do not apply this patch, and instead ask the PM subsystem
> maintainers if they would be willing to implement what we need.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/9] i2c: i801: Remove not needed check for PCI_COMMAND_INTX_DISABLE

[Jean Delvare]

On Fri, 06 Aug 2021 23:15:51 +0200, Heiner Kallweit wrote:
> do_pci_enable_device() takes care that PCI_COMMAND_INTX_DISABLE
> is cleared if a legacy interrupt is used.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index f56060fcf..7fa06b85f 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1827,19 +1827,12 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  		priv->features &= ~FEATURE_IRQ;
>  
>  	if (priv->features & FEATURE_IRQ) {
> -		u16 pcictl, pcists;
> +		u16 pcists;
>  
>  		/* Complain if an interrupt is already pending */
>  		pci_read_config_word(priv->pci_dev, PCI_STATUS, &pcists);
>  		if (pcists & PCI_STATUS_INTERRUPT)
>  			dev_warn(&dev->dev, "An interrupt is pending!\n");
> -
> -		/* Check if interrupts have been disabled */
> -		pci_read_config_word(priv->pci_dev, PCI_COMMAND, &pcictl);
> -		if (pcictl & PCI_COMMAND_INTX_DISABLE) {
> -			dev_info(&dev->dev, "Interrupts are disabled\n");
> -			priv->features &= ~FEATURE_IRQ;
> -		}
>  	}
>  
>  	if (priv->features & FEATURE_IRQ) {

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

As said before, I'm not sure, but let's apply that and see if anybody
complains.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2 8/9] i2c: i801: Improve register_dell_lis3lv02d_i2c_device

[Jean Delvare]

Hi Heiner, Andy,

On Wed, 11 Aug 2021 23:05:06 +0200, Heiner Kallweit wrote:
> On 11.08.2021 17:52, Andy Shevchenko wrote:
> > On Fri, Aug 06, 2021 at 11:18:05PM +0200, Heiner Kallweit wrote:  
> >> - Use an initializer for struct i2c_board_info info
> >> - Use dmi_match()
> >> - Simplify loop logic  
> > 
> > I'm wondering if changing this to a DMI match table will give better result.
> > 
> > Something like
> > (Sorry I forgot APIs, but plenty of examples are under PDx86: drivers/platform/x86):
> > 
> > struct dmi_..._id *id;
> > 
> > id = dmi_..._match();
> > if (!id) {
> > 	pci_warn();
> > 	return;
> > }
> > 
> > i2c_new_client_device(...);
> 
> We could do something like the following. Whether it's better may be a
> question of personal taste. I have no strong opinion here and would leave
> it to Jean.
> 
> const struct dmi_system_id lis3_id_table[] = {
>         {
>                 .driver_data = (void *)0x29,
>                 .matches = {
>                         DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5550"),
>                 },
>         },
> 	...
> 
> dmi_system_id *id = dmi_first_match(lis3_id_table);
> if (id)
> 	i2c_new_client_device(..., (unsigned int)id->driver_data;
> else
> 	lament()
> 

I gave it an actual try, and must say I'm not convinced. Heiner's patch
is -6 lines of source code and +1 byte on the binary (according to
scripts/bloat-o-meter - ls -l disagrees, don't ask me why). My
suggested alternative (as discussed in the v1 of this patch set,
basically Heiner's patch minus the removal of dmi_product_name) is -3
lines of source code and +17 bytes on the binary, but should be faster
than Heiner's version.

Andy's approach results in an overall increase of the source code by 29
lines and +2582 bytes on the binary. Sure, if you break it down, it's
+2624 data and -42 code, so it does "simplify the code", but that's too
high a price to pay for a marginal code simplification. It also has the
downside of reintroducing a cast from int to pointer and back,
something we were trying to get rid of in another patch of this series.
This could of course be avoided but this would make the patch even
larger.

So thanks, but no thanks. Just because an API exists does not mean you
have to use it in all cases.

I stand on my original position, let's stick to dmi_get_system_info() +
strcmp() as the driver did originally. In other words, don't change
code that has been working for years when the alternatives bring no
clear benefit.

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2 1/9] i2c: i801: Improve disabling runtime pm

[Heiner Kallweit]

On 26.08.2021 16:00, Jean Delvare wrote:
> Hi Wolfram,
> 
> On Tue, 17 Aug 2021 22:15:58 +0200, Wolfram Sang wrote:
>>>>> I dunno if it's being discussed, but with this you effectively allow user to
>>>>> override the setting. It may screw things up AFAIU the comment above.
>>>>
>>>> No, this hasn't been discussed. At least not now. Thanks for the hint.
>>>> This attribute is writable for the root user, so we could argue that
>>>> the root user has several options to break the system anyway.  
> 
> This is something we hear frequently when people don't want to address
> problems in their code, but that's not enough to convince me ;-)
> 
>>> But it will mean the side effect on this driver and typical (root-run) system
>>> application (systemd like?) should care now the knowledge about this
>>> side-effect. I do not think it is desired behaviour. But I'm not a maintainer
>>> and I commented here just to make everybody understand the consequences of the
>>> change.  
> 
> Is systemd going to actually make any change to that attribute? I'm no
> systemd expert, but I can't see any option in the configuration files
> that would be related to autosuspend.
> 
>> Jean, are you still fine with this patch then?
> 
> My original position was that there are a few other drivers already
> doing "this". It's not like we are doing something completely new and
> using an API in a way it had never been used before, so it can't be
> that bad.
> 
> On the other hand, after taking a closer look, I'm not fully certain
> that "this" is exactly the same in all these drivers. For example, in
> blk-pm.c, pm_runtime_set_autosuspend_delay() is being called with value
> -1 initially, but with the idea that someone else (device driver, user)
> may set a positive value later. It's not a permanent disable. The
> 8250_omap driver, however, seems to match the i2c-i801 driver here (I
> say "seems" because honestly I'm not sure I fully understand the
> comments there, but my understanding is that at least in some
> situations, enabling autosuspend later would cause problems).
> 
> That being said, it starts looking like a problem for the PM subsystem
> maintainers. Basically Heiner is trying to move away from an API which
> requires cleaning up on driver removal. This is definitely the
> direction we are collectively taking for years now (the whole devm_*
> family of functions is about exactly that). So it's considered a good
> thing.
> 
> If pm_runtime_set_autosuspend_delay() is not suitable for the task then
> maybe we need a better API. I will admit I'm at a loss when it comes to
> the many pm_runtime_* calls, I'm not going to claim I fully understand
> what each of them is doing exactly. But don't we want to simply call
> pm_runtime_dont_use_autosuspend() here?
> 
> If not and there's no suitable API for the task at the moment, then
> better do not apply this patch, and instead ask the PM subsystem
> maintainers if they would be willing to implement what we need.
> 
To follow-up on this: This patch has been applied already. Therefore,
if decision is to not go with it, it would need to be reverted.

Re: [PATCH v2 1/9] i2c: i801: Improve disabling runtime pm

[Jean Delvare]

On Tue, 31 Aug 2021 08:05:41 +0200, Heiner Kallweit wrote:
> On 26.08.2021 16:00, Jean Delvare wrote:
> > If pm_runtime_set_autosuspend_delay() is not suitable for the task then
> > maybe we need a better API. I will admit I'm at a loss when it comes to
> > the many pm_runtime_* calls, I'm not going to claim I fully understand
> > what each of them is doing exactly. But don't we want to simply call
> > pm_runtime_dont_use_autosuspend() here?
> > 
> > If not and there's no suitable API for the task at the moment, then
> > better do not apply this patch, and instead ask the PM subsystem
> > maintainers if they would be willing to implement what we need.
>
> To follow-up on this: This patch has been applied already. Therefore,
> if decision is to not go with it, it would need to be reverted.

Technically it's not in Linus' tree yet ;-)

I'm still interested to know if pm_runtime_dont_use_autosuspend() is
the right call to use in this situation.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2 1/9] i2c: i801: Improve disabling runtime pm

[Heiner Kallweit]

On 31.08.2021 13:26, Jean Delvare wrote:
> On Tue, 31 Aug 2021 08:05:41 +0200, Heiner Kallweit wrote:
>> On 26.08.2021 16:00, Jean Delvare wrote:
>>> If pm_runtime_set_autosuspend_delay() is not suitable for the task then
>>> maybe we need a better API. I will admit I'm at a loss when it comes to
>>> the many pm_runtime_* calls, I'm not going to claim I fully understand
>>> what each of them is doing exactly. But don't we want to simply call
>>> pm_runtime_dont_use_autosuspend() here?
>>>
>>> If not and there's no suitable API for the task at the moment, then
>>> better do not apply this patch, and instead ask the PM subsystem
>>> maintainers if they would be willing to implement what we need.
>>
>> To follow-up on this: This patch has been applied already. Therefore,
>> if decision is to not go with it, it would need to be reverted.
> 
> Technically it's not in Linus' tree yet ;-)
> 
> I'm still interested to know if pm_runtime_dont_use_autosuspend() is
> the right call to use in this situation.
> 
I don't think so. It disable auto-suspending, but leaves "normal"
runtime-suspending active. Calling pm_runtime_disable() may be an
alternative.
Or we use the following to re-establish the old behavior with a little
less overhead. Getting the mutex isn't needed here because the PCI
core increments the rpm usage_count before calling the remove() hook.

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 1f929e6c3..b5723d946 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1623,7 +1623,7 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits,
 		 * BIOS is accessing the host controller so prevent it from
 		 * suspending automatically from now on.
 		 */
-		pm_runtime_set_autosuspend_delay(&pdev->dev, -1);
+		pm_runtime_get_sync(&pdev->dev);
 	}
 
 	if ((function & ACPI_IO_MASK) == ACPI_READ)
@@ -1891,7 +1891,9 @@ static void i801_remove(struct pci_dev *dev)
 	struct i801_priv *priv = pci_get_drvdata(dev);
 
 	pm_runtime_forbid(&dev->dev);
-	pm_runtime_get_noresume(&dev->dev);
+	/* if acpi_reserved is set then usage_count is incremented already */
+	if (!priv->acpi_reserved)
+		pm_runtime_get_noresume(&dev->dev);
 
 	i801_disable_host_notify(priv);
 	i801_del_mux(priv);
-- 
2.33.0

Re: [PATCH v2 1/9] i2c: i801: Improve disabling runtime pm

[Heiner Kallweit]

On 31.08.2021 22:43, Heiner Kallweit wrote:
> On 31.08.2021 13:26, Jean Delvare wrote:
>> On Tue, 31 Aug 2021 08:05:41 +0200, Heiner Kallweit wrote:
>>> On 26.08.2021 16:00, Jean Delvare wrote:
>>>> If pm_runtime_set_autosuspend_delay() is not suitable for the task then
>>>> maybe we need a better API. I will admit I'm at a loss when it comes to
>>>> the many pm_runtime_* calls, I'm not going to claim I fully understand
>>>> what each of them is doing exactly. But don't we want to simply call
>>>> pm_runtime_dont_use_autosuspend() here?
>>>>
>>>> If not and there's no suitable API for the task at the moment, then
>>>> better do not apply this patch, and instead ask the PM subsystem
>>>> maintainers if they would be willing to implement what we need.
>>>
>>> To follow-up on this: This patch has been applied already. Therefore,
>>> if decision is to not go with it, it would need to be reverted.
>>
>> Technically it's not in Linus' tree yet ;-)
>>
>> I'm still interested to know if pm_runtime_dont_use_autosuspend() is
>> the right call to use in this situation.
>>
> I don't think so. It disable auto-suspending, but leaves "normal"
> runtime-suspending active. Calling pm_runtime_disable() may be an
> alternative.
> Or we use the following to re-establish the old behavior with a little
> less overhead. Getting the mutex isn't needed here because the PCI
> core increments the rpm usage_count before calling the remove() hook.
> 
Just figured out that what I proposed wasn't fully correct. We should
only access priv->acpi_reserved once we're sure the ACPI io handler
can't run in parallel.
Small disclaimer: I'm not fully sure how acpi_remove_address_space_handler()
behaves if it's called whilst the handler is running.
IOW: Whether we can be sure that after the call to acpi_remove_address_space_handler()
the handler isn't running.

On a sidenote:
At least the call to pm_runtime_forbid() isn't needed because runtime pm
is disabled anyway and the calls to pm_runtime_forbid/allow don't
have to be balanced.


diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 1f929e6c3..6394c8340 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1623,7 +1623,7 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits,
 		 * BIOS is accessing the host controller so prevent it from
 		 * suspending automatically from now on.
 		 */
-		pm_runtime_set_autosuspend_delay(&pdev->dev, -1);
+		pm_runtime_get_sync(&pdev->dev);
 	}
 
 	if ((function & ACPI_IO_MASK) == ACPI_READ)
@@ -1890,9 +1890,6 @@ static void i801_remove(struct pci_dev *dev)
 {
 	struct i801_priv *priv = pci_get_drvdata(dev);
 
-	pm_runtime_forbid(&dev->dev);
-	pm_runtime_get_noresume(&dev->dev);
-
 	i801_disable_host_notify(priv);
 	i801_del_mux(priv);
 	i2c_del_adapter(&priv->adapter);
@@ -1901,6 +1898,10 @@ static void i801_remove(struct pci_dev *dev)
 
 	platform_device_unregister(priv->tco_pdev);
 
+	pm_runtime_forbid(&dev->dev);
+	/* if acpi_reserved is set then usage_count is incremented already */
+	if (!priv->acpi_reserved)
+		pm_runtime_get_noresume(&dev->dev);
 	/*
 	 * 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.33.0

Re: [PATCH v2 4/9] i2c: i801: Improve is_dell_system_with_lis3lv02d

[Wolfram Sang]

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

On Fri, Aug 06, 2021 at 11:15:15PM +0200, Heiner Kallweit wrote:
> Replace the ugly cast of the return_value pointer with proper usage.
> In addition use dmi_match() instead of open-coding it.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Applied to for-next, thanks!


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

Re: [PATCH v2 5/9] i2c: i801: Remove not needed check for PCI_COMMAND_INTX_DISABLE

[Wolfram Sang]

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

On Fri, Aug 06, 2021 at 11:15:51PM +0200, Heiner Kallweit wrote:
> do_pci_enable_device() takes care that PCI_COMMAND_INTX_DISABLE
> is cleared if a legacy interrupt is used.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Applied to for-next, thanks!


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

Re: [PATCH v2 6/9] i2c: i801: Improve i801_acpi_probe/remove functions

[Wolfram Sang]

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

On Fri, Aug 06, 2021 at 11:16:33PM +0200, Heiner Kallweit wrote:
> By using ACPI_HANDLE() the handler argument can be retrieved directly.
> Both address space handler functions check the handler argument and
> return an error if it's NULL. This allows to further simplify the code.
> 
> Reviewed-by: Jean Delvare <jdelvare@suse.de>
> Tested-by: Jean Delvare <jdelvare@suse.de>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Applied to for-next, thanks!


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

Re: [PATCH v2 7/9] i2c: i801: Improve i801_add_mux

[Wolfram Sang]

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

On Fri, Aug 06, 2021 at 11:17:10PM +0200, Heiner Kallweit wrote:
> The return value of i801_add_mux() isn't used, so let's change it to void.
> In addition remove the not needed cast to struct gpiod_lookup.
> GPIO_LOOKUP() uses GPIO_LOOKUP_IDX() that includes this cast.
> 
> Reviewed-by: Jean Delvare <jdelvare@suse.de>
> Tested-by: Jean Delvare <jdelvare@suse.de>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Applied to for-next, thanks!


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

Re: [PATCH v2 8/9] i2c: i801: Improve register_dell_lis3lv02d_i2c_device

[Wolfram Sang]

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


> I stand on my original position, let's stick to dmi_get_system_info() +
> strcmp() as the driver did originally. In other words, don't change
> code that has been working for years when the alternatives bring no
> clear benefit.

So, I can skip this patch and continue with patch 9?


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

Re: [PATCH v2 8/9] i2c: i801: Improve register_dell_lis3lv02d_i2c_device

[Jean Delvare]

On Wed, 29 Sep 2021 22:11:20 +0200, Wolfram Sang wrote:
> > I stand on my original position, let's stick to dmi_get_system_info() +
> > strcmp() as the driver did originally. In other words, don't change
> > code that has been working for years when the alternatives bring no
> > clear benefit.  
> 
> So, I can skip this patch and continue with patch 9?

Yes please. There may be a subset of it that can still be applied, we
can look into it once the dust has settled.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2 9/9] i2c: i801: Improve handling platform data for tco device

[Wolfram Sang]

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

On Fri, Aug 06, 2021 at 11:18:40PM +0200, Heiner Kallweit wrote:
> The platform data structures are used in the respective i801_add_tco
> functions only. Therefore we can make the definitions local to these
> functions.
> 
> Reviewed-by: Jean Delvare <jdelvare@suse.de>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Hmm, this doesn't apply on i2c/for-mergewindow. Did I miss a patch?


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

Re: [PATCH v2 9/9] i2c: i801: Improve handling platform data for tco device

[Wolfram Sang]

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

On Sat, Oct 02, 2021 at 09:44:55AM +0200, Wolfram Sang wrote:
> On Fri, Aug 06, 2021 at 11:18:40PM +0200, Heiner Kallweit wrote:
> > The platform data structures are used in the respective i801_add_tco
> > functions only. Therefore we can make the definitions local to these
> > functions.
> > 
> > Reviewed-by: Jean Delvare <jdelvare@suse.de>
> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> 
> Hmm, this doesn't apply on i2c/for-mergewindow. Did I miss a patch?

Since a lot of other i801 patches have been merged meanwhile, I'd need
this patch rebased if you still want it to be applied. Thanks for all
your i801 work!


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

[PATCH v3] i2c: i801: Improve handling platform data for tco device

[Heiner Kallweit]

The platform data structures are used in the respective i801_add_tco
functions only. Therefore we can make the definitions local to these
functions.

Reviewed-by: Jean Delvare <jdelvare@suse.de>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v3:
- rebased
---
 drivers/i2c/busses/i2c-i801.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 8c0695956..cb6e55758 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1463,15 +1463,14 @@ static inline unsigned int i801_get_adapter_class(struct i801_priv *priv)
 }
 #endif
 
-static const struct itco_wdt_platform_data spt_tco_platform_data = {
-	.name = "Intel PCH",
-	.version = 4,
-};
-
 static struct platform_device *
 i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
 		 struct resource *tco_res)
 {
+	static const struct itco_wdt_platform_data pldata = {
+		.name = "Intel PCH",
+		.version = 4,
+	};
 	struct resource *res;
 	unsigned int devfn;
 	u64 base64_addr;
@@ -1514,22 +1513,20 @@ i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
 	res->flags = IORESOURCE_MEM;
 
 	return platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
-					tco_res, 2, &spt_tco_platform_data,
-					sizeof(spt_tco_platform_data));
+					tco_res, 2, &pldata, sizeof(pldata));
 }
 
-static const struct itco_wdt_platform_data cnl_tco_platform_data = {
-	.name = "Intel PCH",
-	.version = 6,
-};
-
 static struct platform_device *
 i801_add_tco_cnl(struct i801_priv *priv, struct pci_dev *pci_dev,
 		 struct resource *tco_res)
 {
-	return platform_device_register_resndata(&pci_dev->dev,
-			"iTCO_wdt", -1, tco_res, 1, &cnl_tco_platform_data,
-			sizeof(cnl_tco_platform_data));
+	static const struct itco_wdt_platform_data pldata = {
+		.name = "Intel PCH",
+		.version = 6,
+	};
+
+	return platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
+						 tco_res, 1, &pldata, sizeof(pldata));
 }
 
 static void i801_add_tco(struct i801_priv *priv)
-- 
2.34.1

Re: [PATCH v2 9/9] i2c: i801: Improve handling platform data for tco device

[Heiner Kallweit]

On 29.11.2021 09:58, Wolfram Sang wrote:
> On Sat, Oct 02, 2021 at 09:44:55AM +0200, Wolfram Sang wrote:
>> On Fri, Aug 06, 2021 at 11:18:40PM +0200, Heiner Kallweit wrote:
>>> The platform data structures are used in the respective i801_add_tco
>>> functions only. Therefore we can make the definitions local to these
>>> functions.
>>>
>>> Reviewed-by: Jean Delvare <jdelvare@suse.de>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>
>> Hmm, this doesn't apply on i2c/for-mergewindow. Did I miss a patch?
> 
> Since a lot of other i801 patches have been merged meanwhile, I'd need
> this patch rebased if you still want it to be applied. Thanks for all
> your i801 work!
> 
I just sent a rebased version.