[PATCH v1 00/15] i2c-multi-instantiate: Adapt for INT3515 and alike

[PATCH v1 00/15] i2c-multi-instantiate: Adapt for INT3515 and alike

[Andy Shevchenko]

Currently i2c-multi-instantiate driver does not support the case
of INT3515 USB PD device, where:

- Interrupt() is used instead of GpioInt()
- All slaves may be the same from IP point of view
- There can be variadic amount of slaves

This series is addressing all above.

Notes:
- series has been smoke tested (Heikki would do BAT or more) on Coffee Lake system
- patch for BOSC0200 might be considered as RFC since the modalias potential issue

The idea is to push this either through PDx86 tree (needs Rafael's ACKs) or ACPI.
In any case it needs tags from Heikki, Hans, Mika, Wolfram and Jonathan.

Testing and comments are warmly welcome.

Andy Shevchenko (15):
  platform/x86: intel_cht_int33fe: Remove duplicate NULL check
  platform/x86: intel_cht_int33fe: Accept errors of
    i2c_acpi_new_device()
  platform/x86: i2c-multi-instantiate: Accept errors of
    i2c_acpi_new_device()
  platform/x86: i2c-mutli-instantiate: Defer probe when no adapter found
  i2c: acpi: Return error pointers from i2c_acpi_new_device()
  i2c: acpi: Assign fwnode for devices created via i2c_acpi_new_device()
  i2c: acpi: Use ACPI_FAILURE instead of !ACPI_SUCCESS
  i2c: acpi: Introduce i2c_acpi_get_i2c_resource() helper
  platform/x86: i2c-multi-instantiate: Count I2cSerialBus() resources
  platform/x86: i2c-multi-instantiate: Distinguish IRQ resource type
  platform/x86: i2c-multi-instantiate: Introduce IOAPIC IRQ support
  platform/x86: i2c-multi-instantiate: Allow to have same slaves
  ACPI / scan: Create platform device for INT3515 ACPI nodes
  ACPI / scan: Create platform device for BOSC0200 ACPI nodes
  iio: inv_mpu6050: Use i2c_acpi_get_i2c_resource() helper

 drivers/acpi/scan.c                          |  2 +
 drivers/i2c/i2c-core-acpi.c                  | 43 +++++----
 drivers/iio/accel/bmc150-accel-i2c.c         |  1 -
 drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c   | 16 ++--
 drivers/platform/x86/i2c-multi-instantiate.c | 99 ++++++++++++++++----
 drivers/platform/x86/intel_cht_int33fe.c     | 34 +++++--
 drivers/usb/typec/tps6598x.c                 |  8 +-
 include/linux/acpi.h                         | 11 +++
 8 files changed, 158 insertions(+), 56 deletions(-)

-- 
2.19.1

[PATCH v1 01/15] platform/x86: intel_cht_int33fe: Remove duplicate NULL check

[Andy Shevchenko]

Since i2c_unregister_device() became NULL-aware we may remove duplicate
NULL check.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/intel_cht_int33fe.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
index 464fe93657b5..431151d4e611 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -213,8 +213,7 @@ static int cht_int33fe_probe(struct platform_device *pdev)
 	i2c_unregister_device(data->fusb302);
 
 out_unregister_max17047:
-	if (data->max17047)
-		i2c_unregister_device(data->max17047);
+	i2c_unregister_device(data->max17047);
 
 	device_connections_remove(data->connections);
 
@@ -227,8 +226,7 @@ static int cht_int33fe_remove(struct platform_device *pdev)
 
 	i2c_unregister_device(data->pi3usb30532);
 	i2c_unregister_device(data->fusb302);
-	if (data->max17047)
-		i2c_unregister_device(data->max17047);
+	i2c_unregister_device(data->max17047);
 
 	device_connections_remove(data->connections);
 
-- 
2.19.1

[PATCH v1 02/15] platform/x86: intel_cht_int33fe: Accept errors of i2c_acpi_new_device()

[Andy Shevchenko]

In the future i2c_acpi_new_device() will return error pointer in some cases.
Prepare i2c-multi-instantiate driver to support that.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/intel_cht_int33fe.c | 28 +++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
index 431151d4e611..367d6e304ade 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -168,8 +168,14 @@ static int cht_int33fe_probe(struct platform_device *pdev)
 		board_info.dev_name = "max17047";
 		board_info.properties = max17047_props;
 		data->max17047 = i2c_acpi_new_device(dev, 1, &board_info);
-		if (!data->max17047)
-			return -EPROBE_DEFER; /* Wait for i2c-adapter to load */
+		if (IS_ERR(data->max17047))
+			ret = PTR_ERR(data->max17047);
+		else if (!data->max17047)
+			ret = -EPROBE_DEFER; /* Wait for i2c-adapter to load */
+		else
+			ret = 0;
+		if (ret)
+			return ret;
 	}
 
 	data->connections[0].endpoint[0] = "port0";
@@ -194,7 +200,13 @@ static int cht_int33fe_probe(struct platform_device *pdev)
 	board_info.irq = fusb302_irq;
 
 	data->fusb302 = i2c_acpi_new_device(dev, 2, &board_info);
-	if (!data->fusb302)
+	if (IS_ERR(data->fusb302))
+		ret = PTR_ERR(data->fusb302);
+	else if (!data->fusb302)
+		ret = -EPROBE_DEFER; /* Wait for the i2c-adapter to load */
+	else
+		ret = 0;
+	if (ret)
 		goto out_unregister_max17047;
 
 	memset(&board_info, 0, sizeof(board_info));
@@ -202,7 +214,13 @@ static int cht_int33fe_probe(struct platform_device *pdev)
 	strlcpy(board_info.type, "pi3usb30532", I2C_NAME_SIZE);
 
 	data->pi3usb30532 = i2c_acpi_new_device(dev, 3, &board_info);
-	if (!data->pi3usb30532)
+	if (IS_ERR(data->pi3usb30532))
+		ret = PTR_ERR(data->pi3usb30532);
+	else if (!data->pi3usb30532)
+		ret = -EPROBE_DEFER; /* Wait for the i2c-adapter to load */
+	else
+		ret = 0;
+	if (ret)
 		goto out_unregister_fusb302;
 
 	platform_set_drvdata(pdev, data);
@@ -217,7 +235,7 @@ static int cht_int33fe_probe(struct platform_device *pdev)
 
 	device_connections_remove(data->connections);
 
-	return -EPROBE_DEFER; /* Wait for the i2c-adapter to load */
+	return ret;
 }
 
 static int cht_int33fe_remove(struct platform_device *pdev)
-- 
2.19.1

[PATCH v1 03/15] platform/x86: i2c-multi-instantiate: Accept errors of i2c_acpi_new_device()

[Andy Shevchenko]

In the future i2c_acpi_new_device() will return error pointer in some cases.
Prepare i2c-multi-instantiate driver to support that.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/i2c-multi-instantiate.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
index 5456581b473c..e3345da82c84 100644
--- a/drivers/platform/x86/i2c-multi-instantiate.c
+++ b/drivers/platform/x86/i2c-multi-instantiate.c
@@ -72,9 +72,14 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
 			board_info.irq = ret;
 		}
 		multi->clients[i] = i2c_acpi_new_device(dev, i, &board_info);
-		if (!multi->clients[i]) {
-			dev_err(dev, "Error creating i2c-client, idx %d\n", i);
+		if (IS_ERR(multi->clients[i]))
+			ret = PTR_ERR(multi->clients[i]);
+		else if (!multi->clients[i])
 			ret = -ENODEV;
+		else
+			ret = 0;
+		if (ret) {
+			dev_err(dev, "Error creating i2c-client, idx %d\n", i);
 			goto error;
 		}
 	}
-- 
2.19.1

[PATCH v1 04/15] platform/x86: i2c-mutli-instantiate: Defer probe when no adapter found

[Andy Shevchenko]

Likewise the rest of the i2c_acpi_new_device() users, defer the probe
of the i2c-multi-intantiate driver in case adapter is not yet found.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/i2c-multi-instantiate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
index e3345da82c84..16a0eabe1e31 100644
--- a/drivers/platform/x86/i2c-multi-instantiate.c
+++ b/drivers/platform/x86/i2c-multi-instantiate.c
@@ -75,7 +75,7 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
 		if (IS_ERR(multi->clients[i]))
 			ret = PTR_ERR(multi->clients[i]);
 		else if (!multi->clients[i])
-			ret = -ENODEV;
+			ret = -EPROBE_DEFER; /* Wait for i2c-adapter to load */
 		else
 			ret = 0;
 		if (ret) {
-- 
2.19.1

[PATCH v1 05/15] i2c: acpi: Return error pointers from i2c_acpi_new_device()

[Andy Shevchenko]

The caller would like to know the reason why the i2c_acpi_new_device() fails.
For example, if adapter is not available, it might be in the future and we
would like to re-probe the clients again. But at the same time we would like to
bail out if the error seems unrecoverable, such as out of memory condition.
To achieve this, return error pointer in some cases.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/i2c-core-acpi.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index 32affd3fa8bd..af4b5bd5d973 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -387,6 +387,7 @@ struct notifier_block i2c_acpi_notifier = {
  * Also see i2c_new_device, which this function calls to create the i2c-client.
  *
  * Returns a pointer to the new i2c-client, or NULL if the adapter is not found.
+ * In some cases might return an error pointer.
  */
 struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
 				       struct i2c_board_info *info)
@@ -399,7 +400,7 @@ struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
 
 	adev = ACPI_COMPANION(dev);
 	if (!adev)
-		return NULL;
+		return ERR_PTR(-ENODEV);
 
 	memset(&lookup, 0, sizeof(lookup));
 	lookup.info = info;
@@ -409,9 +410,11 @@ struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
 	ret = acpi_dev_get_resources(adev, &resource_list,
 				     i2c_acpi_fill_info, &lookup);
 	acpi_dev_free_resource_list(&resource_list);
+	if (ret < 0)
+		return ERR_PTR(ret);
 
-	if (ret < 0 || !info->addr)
-		return NULL;
+	if (!info->addr)
+		return ERR_PTR(-EADDRNOTAVAIL);
 
 	adapter = i2c_acpi_find_adapter_by_handle(lookup.adapter_handle);
 	if (!adapter)
-- 
2.19.1

[PATCH v1 06/15] i2c: acpi: Assign fwnode for devices created via i2c_acpi_new_device()

[Andy Shevchenko]

i2c_acpi_new_device() doesn't assign fwnode like it's done, for example,
in i2c_acpi_register_devices() path.

Assign fwnode in i2c_acpi_new_device() as it's done elsewhere.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/i2c-core-acpi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index af4b5bd5d973..478862abb82a 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -420,6 +420,7 @@ struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
 	if (!adapter)
 		return NULL;
 
+	info->fwnode = acpi_fwnode_handle(adev);
 	return i2c_new_device(adapter, info);
 }
 EXPORT_SYMBOL_GPL(i2c_acpi_new_device);
-- 
2.19.1

[PATCH v1 07/15] i2c: acpi: Use ACPI_FAILURE instead of !ACPI_SUCCESS

[Andy Shevchenko]

Convert to use ACPI_FAILURE instead of !ACPI_SUCCESS.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/i2c-core-acpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index 478862abb82a..99553b1469b4 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -65,7 +65,7 @@ static int i2c_acpi_fill_info(struct acpi_resource *ares, void *data)
 	status = acpi_get_handle(lookup->device_handle,
 				 sb->resource_source.string_ptr,
 				 &lookup->adapter_handle);
-	if (!ACPI_SUCCESS(status))
+	if (ACPI_FAILURE(status))
 		return 1;
 
 	info->addr = sb->slave_address;
-- 
2.19.1

[PATCH v1 08/15] i2c: acpi: Introduce i2c_acpi_get_i2c_resource() helper

[Andy Shevchenko]

Besides current two users one more is coming. Definitely makes sense to
introduce a helper.

No functional change intended.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/i2c-core-acpi.c | 31 +++++++++++++++++++------------
 include/linux/acpi.h        | 11 +++++++++++
 2 files changed, 30 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index 99553b1469b4..59b8b191d752 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -45,6 +45,23 @@ struct i2c_acpi_lookup {
 	u32 min_speed;
 };
 
+bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares,
+			       struct acpi_resource_i2c_serialbus **i2c)
+{
+	struct acpi_resource_i2c_serialbus *sb;
+
+	if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
+		return false;
+
+	sb = &ares->data.i2c_serial_bus;
+	if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C)
+		return false;
+
+	*i2c = sb;
+	return true;
+}
+EXPORT_SYMBOL_GPL(i2c_acpi_get_i2c_resource);
+
 static int i2c_acpi_fill_info(struct acpi_resource *ares, void *data)
 {
 	struct i2c_acpi_lookup *lookup = data;
@@ -52,11 +69,7 @@ static int i2c_acpi_fill_info(struct acpi_resource *ares, void *data)
 	struct acpi_resource_i2c_serialbus *sb;
 	acpi_status status;
 
-	if (info->addr || ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
-		return 1;
-
-	sb = &ares->data.i2c_serial_bus;
-	if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C)
+	if (info->addr || !i2c_acpi_get_i2c_resource(ares, &sb))
 		return 1;
 
 	if (lookup->index != -1 && lookup->n++ != lookup->index)
@@ -529,13 +542,7 @@ i2c_acpi_space_handler(u32 function, acpi_physical_address command,
 		goto err;
 	}
 
-	if (!value64 || ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS) {
-		ret = AE_BAD_PARAMETER;
-		goto err;
-	}
-
-	sb = &ares->data.i2c_serial_bus;
-	if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C) {
+	if (!value64 || !i2c_acpi_get_i2c_resource(ares, &sb)) {
 		ret = AE_BAD_PARAMETER;
 		goto err;
 	}
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index ed80f147bd50..6afc6e3c4c5c 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1054,6 +1054,17 @@ static inline int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
 }
 #endif
 
+#if defined(CONFIG_ACPI) && IS_ENABLED(CONFIG_I2C)
+bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares,
+			       struct acpi_resource_i2c_serialbus **i2c);
+#else
+static inline bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares,
+					     struct acpi_resource_i2c_serialbus **i2c)
+{
+	return false;
+}
+#endif
+
 /* Device properties */
 
 #ifdef CONFIG_ACPI
-- 
2.19.1

[PATCH v1 09/15] platform/x86: i2c-multi-instantiate: Count I2cSerialBus() resources

[Andy Shevchenko]

Instead of relying on hard coded and thus expected number of I2C clients,
count the real amount provided by firmware.

This allows to support non-fixed amount of the slaves.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/i2c-multi-instantiate.c | 34 ++++++++++++++++++--
 1 file changed, 31 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
index 16a0eabe1e31..d17686e10d08 100644
--- a/drivers/platform/x86/i2c-multi-instantiate.c
+++ b/drivers/platform/x86/i2c-multi-instantiate.c
@@ -12,6 +12,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/types.h>
 
 struct i2c_inst_data {
 	const char *type;
@@ -23,6 +24,31 @@ struct i2c_multi_inst_data {
 	struct i2c_client *clients[0];
 };
 
+static int i2c_multi_inst_count(struct acpi_resource *ares, void *data)
+{
+	struct acpi_resource_i2c_serialbus *sb;
+	int *count = data;
+
+	if (i2c_acpi_get_i2c_resource(ares, &sb))
+		*count = *count + 1;
+
+	return 1;
+}
+
+static int i2c_multi_inst_count_resources(struct acpi_device *adev)
+{
+	LIST_HEAD(r);
+	int count = 0;
+	int ret;
+
+	ret = acpi_dev_get_resources(adev, &r, i2c_multi_inst_count, &count);
+	if (ret < 0)
+		return ret;
+
+	acpi_dev_free_resource_list(&r);
+	return count;
+}
+
 static int i2c_multi_inst_probe(struct platform_device *pdev)
 {
 	struct i2c_multi_inst_data *multi;
@@ -44,15 +70,17 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
 	adev = ACPI_COMPANION(dev);
 
 	/* Count number of clients to instantiate */
-	for (i = 0; inst_data[i].type; i++) {}
+	ret = i2c_multi_inst_count_resources(adev);
+	if (ret < 0)
+		return ret;
 
 	multi = devm_kmalloc(dev,
-			offsetof(struct i2c_multi_inst_data, clients[i]),
+			offsetof(struct i2c_multi_inst_data, clients[ret]),
 			GFP_KERNEL);
 	if (!multi)
 		return -ENOMEM;
 
-	multi->num_clients = i;
+	multi->num_clients = ret;
 
 	for (i = 0; i < multi->num_clients; i++) {
 		memset(&board_info, 0, sizeof(board_info));
-- 
2.19.1

[PATCH v1 10/15] platform/x86: i2c-multi-instantiate: Distinguish IRQ resource type

[Andy Shevchenko]

As a preparatory patch switch the driver to distinguish IRQ resource type.
For now, only GpioInt() is supported.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/i2c-multi-instantiate.c | 27 +++++++++++++-------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
index d17686e10d08..336ef28e072d 100644
--- a/drivers/platform/x86/i2c-multi-instantiate.c
+++ b/drivers/platform/x86/i2c-multi-instantiate.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/bits.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
@@ -14,9 +15,14 @@
 #include <linux/platform_device.h>
 #include <linux/types.h>
 
+#define IRQ_RESOURCE_TYPE	GENMASK(1, 0)
+#define IRQ_RESOURCE_NONE	0
+#define IRQ_RESOURCE_GPIO	1
+
 struct i2c_inst_data {
 	const char *type;
-	int gpio_irq_idx;
+	unsigned int flags;
+	int irq_idx;
 };
 
 struct i2c_multi_inst_data {
@@ -88,16 +94,19 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
 		snprintf(name, sizeof(name), "%s-%s", match->id,
 			 inst_data[i].type);
 		board_info.dev_name = name;
-		board_info.irq = 0;
-		if (inst_data[i].gpio_irq_idx != -1) {
-			ret = acpi_dev_gpio_irq_get(adev,
-						    inst_data[i].gpio_irq_idx);
+		switch (inst_data[i].flags & IRQ_RESOURCE_TYPE) {
+		case IRQ_RESOURCE_GPIO:
+			ret = acpi_dev_gpio_irq_get(adev, inst_data[i].irq_idx);
 			if (ret < 0) {
 				dev_err(dev, "Error requesting irq at index %d: %d\n",
-					inst_data[i].gpio_irq_idx, ret);
+					inst_data[i].irq_idx, ret);
 				goto error;
 			}
 			board_info.irq = ret;
+			break;
+		default:
+			board_info.irq = 0;
+			break;
 		}
 		multi->clients[i] = i2c_acpi_new_device(dev, i, &board_info);
 		if (IS_ERR(multi->clients[i]))
@@ -134,9 +143,9 @@ static int i2c_multi_inst_remove(struct platform_device *pdev)
 }
 
 static const struct i2c_inst_data bsg1160_data[]  = {
-	{ "bmc150_accel", 0 },
-	{ "bmc150_magn", -1 },
-	{ "bmg160", -1 },
+	{ "bmc150_accel", IRQ_RESOURCE_GPIO, 0 },
+	{ "bmc150_magn" },
+	{ "bmg160" },
 	{}
 };
 
-- 
2.19.1

[PATCH v1 11/15] platform/x86: i2c-multi-instantiate: Introduce IOAPIC IRQ support

[Andy Shevchenko]

If ACPI table provides an Interrupt() resource we may consider to use it
instead of GpioInt() one.

Here we leave an error condition, when getting IRQ resource, to the driver
to decide how to proceed, because some drivers may consider IRQ resource
optional.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/i2c-multi-instantiate.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
index 336ef28e072d..3334fafb4d8c 100644
--- a/drivers/platform/x86/i2c-multi-instantiate.c
+++ b/drivers/platform/x86/i2c-multi-instantiate.c
@@ -18,6 +18,7 @@
 #define IRQ_RESOURCE_TYPE	GENMASK(1, 0)
 #define IRQ_RESOURCE_NONE	0
 #define IRQ_RESOURCE_GPIO	1
+#define IRQ_RESOURCE_APIC	2
 
 struct i2c_inst_data {
 	const char *type;
@@ -104,6 +105,14 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
 			}
 			board_info.irq = ret;
 			break;
+		case IRQ_RESOURCE_APIC:
+			ret = platform_get_irq(pdev, inst_data[i].irq_idx);
+			if (ret < 0) {
+				dev_dbg(dev, "Error requesting irq at index %d: %d\n",
+					inst_data[i].irq_idx, ret);
+			}
+			board_info.irq = ret;
+			break;
 		default:
 			board_info.irq = 0;
 			break;
-- 
2.19.1

[PATCH v1 12/15] platform/x86: i2c-multi-instantiate: Allow to have same slaves

[Andy Shevchenko]

Currently the driver will not enumerate the devices where I2C slaves
are of the same type.

Add an instance number to make them unique.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/i2c-multi-instantiate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
index 3334fafb4d8c..1e078a961245 100644
--- a/drivers/platform/x86/i2c-multi-instantiate.c
+++ b/drivers/platform/x86/i2c-multi-instantiate.c
@@ -92,8 +92,8 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
 	for (i = 0; i < multi->num_clients; i++) {
 		memset(&board_info, 0, sizeof(board_info));
 		strlcpy(board_info.type, inst_data[i].type, I2C_NAME_SIZE);
-		snprintf(name, sizeof(name), "%s-%s", match->id,
-			 inst_data[i].type);
+		snprintf(name, sizeof(name), "%s-%s.%d", match->id,
+			 inst_data[i].type, i);
 		board_info.dev_name = name;
 		switch (inst_data[i].flags & IRQ_RESOURCE_TYPE) {
 		case IRQ_RESOURCE_GPIO:
-- 
2.19.1

[PATCH v1 13/15] ACPI / scan: Create platform device for INT3515 ACPI nodes

[Andy Shevchenko]

The ACPI device with INT3515 _HID is representing a complex USB PD
hardware infrastructure which includes several I2C slave ICs.

We add an ID to the I2C multi instantiate list to enumerate
all I2C slaves correctly.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/acpi/scan.c                          | 1 +
 drivers/platform/x86/i2c-multi-instantiate.c | 9 +++++++++
 drivers/usb/typec/tps6598x.c                 | 8 ++++----
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index bd1c59fb0e17..e9eda5558c1f 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1541,6 +1541,7 @@ static bool acpi_device_enumeration_by_parent(struct acpi_device *device)
 	static const struct acpi_device_id i2c_multi_instantiate_ids[] = {
 		{"BSG1160", },
 		{"INT33FE", },
+		{"INT3515", },
 		{}
 	};
 
diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
index 1e078a961245..efaf34cbbc7b 100644
--- a/drivers/platform/x86/i2c-multi-instantiate.c
+++ b/drivers/platform/x86/i2c-multi-instantiate.c
@@ -158,12 +158,21 @@ static const struct i2c_inst_data bsg1160_data[]  = {
 	{}
 };
 
+static const struct i2c_inst_data int3515_data[]  = {
+	{ "tps6598x", IRQ_RESOURCE_APIC, 0 },
+	{ "tps6598x", IRQ_RESOURCE_APIC, 1 },
+	{ "tps6598x", IRQ_RESOURCE_APIC, 2 },
+	{ "tps6598x", IRQ_RESOURCE_APIC, 3 },
+	{}
+};
+
 /*
  * Note new device-ids must also be added to i2c_multi_instantiate_ids in
  * drivers/acpi/scan.c: acpi_device_enumeration_by_parent().
  */
 static const struct acpi_device_id i2c_multi_inst_acpi_ids[] = {
 	{ "BSG1160", (unsigned long)bsg1160_data },
+	{ "INT3515", (unsigned long)int3515_data },
 	{ }
 };
 MODULE_DEVICE_TABLE(acpi, i2c_multi_inst_acpi_ids);
diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
index c84c8c189e90..1c0033ad8738 100644
--- a/drivers/usb/typec/tps6598x.c
+++ b/drivers/usb/typec/tps6598x.c
@@ -501,19 +501,19 @@ static int tps6598x_remove(struct i2c_client *client)
 	return 0;
 }
 
-static const struct acpi_device_id tps6598x_acpi_match[] = {
-	{ "INT3515", 0 },
+static const struct i2c_device_id tps6598x_id[] = {
+	{ "tps6598x" },
 	{ }
 };
-MODULE_DEVICE_TABLE(acpi, tps6598x_acpi_match);
+MODULE_DEVICE_TABLE(i2c, tps6598x_id);
 
 static struct i2c_driver tps6598x_i2c_driver = {
 	.driver = {
 		.name = "tps6598x",
-		.acpi_match_table = tps6598x_acpi_match,
 	},
 	.probe_new = tps6598x_probe,
 	.remove = tps6598x_remove,
+	.id_table = tps6598x_id,
 };
 module_i2c_driver(tps6598x_i2c_driver);
 
-- 
2.19.1

[PATCH v1 14/15] ACPI / scan: Create platform device for BOSC0200 ACPI nodes

[Andy Shevchenko]

On some laptops the ACPI device with BOSC0200 _HID is representing
two accelerometers under one node.

We add an ID to the I2C multi instantiate list to enumerate
all I2C slaves correctly.

For reference here is the relevant DSDT blurb from the Yoga 11e:

Device (ACC)
{
	Name (_ADR, Zero)  // _ADR: Address
	Name (_HID, "BOSC0200")  // _HID: Hardware ID
	Name (_CID, "BOSC0200")  // _CID: Compatible ID
	Name (_DDN, "Accelerometer")  // _DDN: DOS Device Name
	Name (_UID, One)  // _UID: Unique ID
	Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
	{
		Name (RBUF, ResourceTemplate ()
		{
			I2cSerialBusV2 (0x0019, ControllerInitiated, 0x00061A80,
				AddressingMode7Bit, "\\_SB.PCI0.I2C3",
				0x00, ResourceConsumer, , Exclusive,
			)
			I2cSerialBusV2 (0x0018, ControllerInitiated, 0x00061A80,
				AddressingMode7Bit, "\\_SB.PCI0.I2C3",
				0x00, ResourceConsumer, , Exclusive,
			)
		})
		Return (RBUF) /* \_SB_.PCI0.I2C3.ACC_._CRS.RBUF */
	}

Reported-by: Jeremy Cline <jeremy@jcline.org>
Cc: Steven Presser <steve@pressers.name>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/acpi/scan.c                          | 1 +
 drivers/iio/accel/bmc150-accel-i2c.c         | 1 -
 drivers/platform/x86/i2c-multi-instantiate.c | 7 +++++++
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index e9eda5558c1f..e46c877fa9f1 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1539,6 +1539,7 @@ static bool acpi_device_enumeration_by_parent(struct acpi_device *device)
 	 * which i2c_device_id to use for each resource.
 	 */
 	static const struct acpi_device_id i2c_multi_instantiate_ids[] = {
+		{"BOSC0200", },
 		{"BSG1160", },
 		{"INT33FE", },
 		{"INT3515", },
diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
index 8ffc308d5fd0..9d22a4d9d568 100644
--- a/drivers/iio/accel/bmc150-accel-i2c.c
+++ b/drivers/iio/accel/bmc150-accel-i2c.c
@@ -64,7 +64,6 @@ static const struct acpi_device_id bmc150_accel_acpi_match[] = {
 	{"BMA250E",	bma250e},
 	{"BMA222E",	bma222e},
 	{"BMA0280",	bma280},
-	{"BOSC0200"},
 	{ },
 };
 MODULE_DEVICE_TABLE(acpi, bmc150_accel_acpi_match);
diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
index efaf34cbbc7b..c90137d30a98 100644
--- a/drivers/platform/x86/i2c-multi-instantiate.c
+++ b/drivers/platform/x86/i2c-multi-instantiate.c
@@ -151,6 +151,12 @@ static int i2c_multi_inst_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct i2c_inst_data bosc0200_data[]  = {
+	{ "bmc150_accel" },
+	{ "bmc150_accel" },
+	{}
+};
+
 static const struct i2c_inst_data bsg1160_data[]  = {
 	{ "bmc150_accel", IRQ_RESOURCE_GPIO, 0 },
 	{ "bmc150_magn" },
@@ -171,6 +177,7 @@ static const struct i2c_inst_data int3515_data[]  = {
  * drivers/acpi/scan.c: acpi_device_enumeration_by_parent().
  */
 static const struct acpi_device_id i2c_multi_inst_acpi_ids[] = {
+	{ "BOSC0200", (unsigned long)bosc0200_data },
 	{ "BSG1160", (unsigned long)bsg1160_data },
 	{ "INT3515", (unsigned long)int3515_data },
 	{ }
-- 
2.19.1

[PATCH v1 15/15] iio: inv_mpu6050: Use i2c_acpi_get_i2c_resource() helper

[Andy Shevchenko]

ACPI provides a generic helper to get I2C Serial Bus resources.
Use it instead of open coded variant.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c
index d78a10403bac..a961b5a06fe6 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c
@@ -91,18 +91,14 @@ static int asus_acpi_get_sensor_info(struct acpi_device *adev,
 
 static int acpi_i2c_check_resource(struct acpi_resource *ares, void *data)
 {
+	struct acpi_resource_i2c_serialbus *sb;
 	u32 *addr = data;
 
-	if (ares->type == ACPI_RESOURCE_TYPE_SERIAL_BUS) {
-		struct acpi_resource_i2c_serialbus *sb;
-
-		sb = &ares->data.i2c_serial_bus;
-		if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_I2C) {
-			if (*addr)
-				*addr |= (sb->slave_address << 16);
-			else
-				*addr = sb->slave_address;
-		}
+	if (i2c_acpi_get_i2c_resource(ares, &sb)) {
+		if (*addr)
+			*addr |= (sb->slave_address << 16);
+		else
+			*addr = sb->slave_address;
 	}
 
 	/* Tell the ACPI core that we already copied this address */
-- 
2.19.1

Re: [PATCH v1 00/15] i2c-multi-instantiate: Adapt for INT3515 and alike

[Hans de Goede]

Hi,

On 20-11-18 16:59, Andy Shevchenko wrote:
> Currently i2c-multi-instantiate driver does not support the case
> of INT3515 USB PD device, where:
> 
> - Interrupt() is used instead of GpioInt()
> - All slaves may be the same from IP point of view
> - There can be variadic amount of slaves
> 
> This series is addressing all above.
> 
> Notes:
> - series has been smoke tested (Heikki would do BAT or more) on Coffee Lake system
> - patch for BOSC0200 might be considered as RFC since the modalias potential issue

Yeah the BOSC0200 patch cannot go upstream as is, that will break the
orientation quirks in hwdb on:

[hans@shalem systemd]$ grep BOSC0200 hwdb/60-sensor.hwdb | wc -l
17
[hans@shalem systemd]$

At least 17 different models laptops / 2-in-1s

> The idea is to push this either through PDx86 tree (needs Rafael's ACKs) or ACPI.
> In any case it needs tags from Heikki, Hans, Mika, Wolfram and Jonathan.

Patches 1-5, 7-13 and 15 are:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

I will send separate replies to:

[PATCH v1 06/15] i2c: acpi: Assign fwnode for devices created via i2c_acpi_new_device()
[PATCH v1 14/15] ACPI / scan: Create platform device for BOSC0200 ACPI nodes

Which are both a NACK from me (in their current state) because they both will
cause regressions.

Regards,

Hans






> 
> Testing and comments are warmly welcome.
> 
> Andy Shevchenko (15):
>    platform/x86: intel_cht_int33fe: Remove duplicate NULL check
>    platform/x86: intel_cht_int33fe: Accept errors of
>      i2c_acpi_new_device()
>    platform/x86: i2c-multi-instantiate: Accept errors of
>      i2c_acpi_new_device()
>    platform/x86: i2c-mutli-instantiate: Defer probe when no adapter found
>    i2c: acpi: Return error pointers from i2c_acpi_new_device()
>    i2c: acpi: Assign fwnode for devices created via i2c_acpi_new_device()
>    i2c: acpi: Use ACPI_FAILURE instead of !ACPI_SUCCESS
>    i2c: acpi: Introduce i2c_acpi_get_i2c_resource() helper
>    platform/x86: i2c-multi-instantiate: Count I2cSerialBus() resources
>    platform/x86: i2c-multi-instantiate: Distinguish IRQ resource type
>    platform/x86: i2c-multi-instantiate: Introduce IOAPIC IRQ support
>    platform/x86: i2c-multi-instantiate: Allow to have same slaves
>    ACPI / scan: Create platform device for INT3515 ACPI nodes
>    ACPI / scan: Create platform device for BOSC0200 ACPI nodes
>    iio: inv_mpu6050: Use i2c_acpi_get_i2c_resource() helper
> 
>   drivers/acpi/scan.c                          |  2 +
>   drivers/i2c/i2c-core-acpi.c                  | 43 +++++----
>   drivers/iio/accel/bmc150-accel-i2c.c         |  1 -
>   drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c   | 16 ++--
>   drivers/platform/x86/i2c-multi-instantiate.c | 99 ++++++++++++++++----
>   drivers/platform/x86/intel_cht_int33fe.c     | 34 +++++--
>   drivers/usb/typec/tps6598x.c                 |  8 +-
>   include/linux/acpi.h                         | 11 +++
>   8 files changed, 158 insertions(+), 56 deletions(-)
> 

Re: [PATCH v1 06/15] i2c: acpi: Assign fwnode for devices created via i2c_acpi_new_device()

[Hans de Goede]

Hi,

On 20-11-18 16:59, Andy Shevchenko wrote:
> i2c_acpi_new_device() doesn't assign fwnode like it's done, for example,
> in i2c_acpi_register_devices() path.
> 
> Assign fwnode in i2c_acpi_new_device() as it's done elsewhere.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

As already mentioned in my reply to the cover letter, a NACK from me for
this one.

The problem is that this causes the fwnode to be shared by all the
"struct device"-s (embedded in the i2c-clients created).

This in itself is not necessarily a problem, but it does become a
problem when combined with using device_add_properties as
intel_cht_int33fe.c does.

Since the info in the ACPI tables for devices which use this
driver is pretty terrible, we add a bunch of properties to notify
the drivers about how the different parts which make up the Type-C
functionality are tied together.

device_add_properties creates a new fwnode and then calls
set_secondary_fwnode. For devices which already have a fwnode
assigned (which the i2c-clients will have after this patch)
set_secondary_fwnode sets dev->fwnode->secondary to the fwnode
which has been newly created to hold the added properties.

Since all i2c-clients instantiated now share dev->fwnode,
the result of this is that all i2c-clients created now will
have the properties of the last i2c-client instantiated for
an acpi-node which describes multiple clients in a single
node.

Also the fwnodes created  for the properties of earlier
instantiated i2c-clients will be leaked.

I see 2 possible solutions here:

1) Changing this patch so that i2c_acpi_new_device() does:

         /*
          * The fwnode can not be shared between instantiated clients when
          * adding device-properties to the instantiated client.
          */
	if (!info->properties)
		info->fwnode = acpi_fwnode_handle(adev);

2) Leave whether info->fwnode should be set or not up to the
caller of i2c_acpi_new_device() (info is already passed in as
a param). E.g. i2c-multi-instantiate.c could do this (for now,
maybe in the future we will also want to add properties for
some HIDs).

I think that 1. is the best solution and this is all kernel internal,
so we can always revisit this if necessary.

Regards,

Hans



> ---
>   drivers/i2c/i2c-core-acpi.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> index af4b5bd5d973..478862abb82a 100644
> --- a/drivers/i2c/i2c-core-acpi.c
> +++ b/drivers/i2c/i2c-core-acpi.c
> @@ -420,6 +420,7 @@ struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
>   	if (!adapter)
>   		return NULL;
>   
> +	info->fwnode = acpi_fwnode_handle(adev);
>   	return i2c_new_device(adapter, info);
>   }
>   EXPORT_SYMBOL_GPL(i2c_acpi_new_device);
> 

Re: [PATCH v1 00/15] i2c-multi-instantiate: Adapt for INT3515 and alike

[Hans de Goede]

Hi,

On 21-11-18 12:27, Hans de Goede wrote:
> Hi,
> 
> On 20-11-18 16:59, Andy Shevchenko wrote:
>> Currently i2c-multi-instantiate driver does not support the case
>> of INT3515 USB PD device, where:
>>
>> - Interrupt() is used instead of GpioInt()
>> - All slaves may be the same from IP point of view
>> - There can be variadic amount of slaves
>>
>> This series is addressing all above.
>>
>> Notes:
>> - series has been smoke tested (Heikki would do BAT or more) on Coffee Lake system
>> - patch for BOSC0200 might be considered as RFC since the modalias potential issue
> 
> Yeah the BOSC0200 patch cannot go upstream as is, that will break the
> orientation quirks in hwdb on:
> 
> [hans@shalem systemd]$ grep BOSC0200 hwdb/60-sensor.hwdb | wc -l
> 17
> [hans@shalem systemd]$
> 
> At least 17 different models laptops / 2-in-1s
> 
>> The idea is to push this either through PDx86 tree (needs Rafael's ACKs) or ACPI.
>> In any case it needs tags from Heikki, Hans, Mika, Wolfram and Jonathan.
> 
> Patches 1-5, 7-13 and 15 are: >
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Correction, I also have some small remarks to patch 9:

[PATCH v1 09/15] platform/x86: i2c-multi-instantiate: Count I2cSerialBus() resources

Which will need to be addressed before you can add my Reviewed-by.

Regards,

Hans




> I will send separate replies to:
> 
> [PATCH v1 06/15] i2c: acpi: Assign fwnode for devices created via i2c_acpi_new_device()
> [PATCH v1 14/15] ACPI / scan: Create platform device for BOSC0200 ACPI nodes
> 
> Which are both a NACK from me (in their current state) because they both will
> cause regressions.
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
> 
>>
>> Testing and comments are warmly welcome.
>>
>> Andy Shevchenko (15):
>>    platform/x86: intel_cht_int33fe: Remove duplicate NULL check
>>    platform/x86: intel_cht_int33fe: Accept errors of
>>      i2c_acpi_new_device()
>>    platform/x86: i2c-multi-instantiate: Accept errors of
>>      i2c_acpi_new_device()
>>    platform/x86: i2c-mutli-instantiate: Defer probe when no adapter found
>>    i2c: acpi: Return error pointers from i2c_acpi_new_device()
>>    i2c: acpi: Assign fwnode for devices created via i2c_acpi_new_device()
>>    i2c: acpi: Use ACPI_FAILURE instead of !ACPI_SUCCESS
>>    i2c: acpi: Introduce i2c_acpi_get_i2c_resource() helper
>>    platform/x86: i2c-multi-instantiate: Count I2cSerialBus() resources
>>    platform/x86: i2c-multi-instantiate: Distinguish IRQ resource type
>>    platform/x86: i2c-multi-instantiate: Introduce IOAPIC IRQ support
>>    platform/x86: i2c-multi-instantiate: Allow to have same slaves
>>    ACPI / scan: Create platform device for INT3515 ACPI nodes
>>    ACPI / scan: Create platform device for BOSC0200 ACPI nodes
>>    iio: inv_mpu6050: Use i2c_acpi_get_i2c_resource() helper
>>
>>   drivers/acpi/scan.c                          |  2 +
>>   drivers/i2c/i2c-core-acpi.c                  | 43 +++++----
>>   drivers/iio/accel/bmc150-accel-i2c.c         |  1 -
>>   drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c   | 16 ++--
>>   drivers/platform/x86/i2c-multi-instantiate.c | 99 ++++++++++++++++----
>>   drivers/platform/x86/intel_cht_int33fe.c     | 34 +++++--
>>   drivers/usb/typec/tps6598x.c                 |  8 +-
>>   include/linux/acpi.h                         | 11 +++
>>   8 files changed, 158 insertions(+), 56 deletions(-)
>>

Re: [PATCH v1 09/15] platform/x86: i2c-multi-instantiate: Count I2cSerialBus() resources

[Hans de Goede]

Hi,

On 20-11-18 16:59, Andy Shevchenko wrote:
> Instead of relying on hard coded and thus expected number of I2C clients,
> count the real amount provided by firmware.
> 
> This allows to support non-fixed amount of the slaves.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   drivers/platform/x86/i2c-multi-instantiate.c | 34 ++++++++++++++++++--
>   1 file changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
> index 16a0eabe1e31..d17686e10d08 100644
> --- a/drivers/platform/x86/i2c-multi-instantiate.c
> +++ b/drivers/platform/x86/i2c-multi-instantiate.c
> @@ -12,6 +12,7 @@
>   #include <linux/kernel.h>
>   #include <linux/module.h>
>   #include <linux/platform_device.h>
> +#include <linux/types.h>
>   
>   struct i2c_inst_data {
>   	const char *type;
> @@ -23,6 +24,31 @@ struct i2c_multi_inst_data {
>   	struct i2c_client *clients[0];
>   };
>   
> +static int i2c_multi_inst_count(struct acpi_resource *ares, void *data)
> +{
> +	struct acpi_resource_i2c_serialbus *sb;
> +	int *count = data;
> +
> +	if (i2c_acpi_get_i2c_resource(ares, &sb))
> +		*count = *count + 1;
> +
> +	return 1;
> +}
> +
> +static int i2c_multi_inst_count_resources(struct acpi_device *adev)
> +{
> +	LIST_HEAD(r);
> +	int count = 0;
> +	int ret;
> +
> +	ret = acpi_dev_get_resources(adev, &r, i2c_multi_inst_count, &count);
> +	if (ret < 0)
> +		return ret;
> +
> +	acpi_dev_free_resource_list(&r);
> +	return count;
> +}
> +
>   static int i2c_multi_inst_probe(struct platform_device *pdev)
>   {
>   	struct i2c_multi_inst_data *multi;
> @@ -44,15 +70,17 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
>   	adev = ACPI_COMPANION(dev);
>   
>   	/* Count number of clients to instantiate */
> -	for (i = 0; inst_data[i].type; i++) {}
> +	ret = i2c_multi_inst_count_resources(adev);
> +	if (ret < 0)
> +		return ret;
>   
>   	multi = devm_kmalloc(dev,
> -			offsetof(struct i2c_multi_inst_data, clients[i]),
> +			offsetof(struct i2c_multi_inst_data, clients[ret]),
>   			GFP_KERNEL);
>   	if (!multi)
>   		return -ENOMEM;
>   
> -	multi->num_clients = i;
> +	multi->num_clients = ret;
>   
>   	for (i = 0; i < multi->num_clients; i++) {
>   		memset(&board_info, 0, sizeof(board_info));

Now you end up indexing inst_data with i without checking that inst_data
holds enough data. Not using all of inst_data in case there are less
I2cSerialBus resources then inst_data entries is fine. But you need to
deal with the case when there are more I2cSerialBus resources then inst_data
entries, either by erroring out or ignoring the extra I2cSerialBus resources.

Also it might be better to make the i2c_multi_inst_count_resources() function
a generic helper under drivers/acpi, so that it can be used in other places,
see my comments to the BOSC0200 patch.

Regards,

Hans

Re: [PATCH v1 14/15] ACPI / scan: Create platform device for BOSC0200 ACPI nodes

[Hans de Goede]

HI,

On 20-11-18 16:59, Andy Shevchenko wrote:
> On some laptops the ACPI device with BOSC0200 _HID is representing
> two accelerometers under one node.
> 
> We add an ID to the I2C multi instantiate list to enumerate
> all I2C slaves correctly.
> 
> For reference here is the relevant DSDT blurb from the Yoga 11e:
> 
> Device (ACC)
> {
> 	Name (_ADR, Zero)  // _ADR: Address
> 	Name (_HID, "BOSC0200")  // _HID: Hardware ID
> 	Name (_CID, "BOSC0200")  // _CID: Compatible ID
> 	Name (_DDN, "Accelerometer")  // _DDN: DOS Device Name
> 	Name (_UID, One)  // _UID: Unique ID
> 	Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
> 	{
> 		Name (RBUF, ResourceTemplate ()
> 		{
> 			I2cSerialBusV2 (0x0019, ControllerInitiated, 0x00061A80,
> 				AddressingMode7Bit, "\\_SB.PCI0.I2C3",
> 				0x00, ResourceConsumer, , Exclusive,
> 			)
> 			I2cSerialBusV2 (0x0018, ControllerInitiated, 0x00061A80,
> 				AddressingMode7Bit, "\\_SB.PCI0.I2C3",
> 				0x00, ResourceConsumer, , Exclusive,
> 			)
> 		})
> 		Return (RBUF) /* \_SB_.PCI0.I2C3.ACC_._CRS.RBUF */
> 	}
> 
> Reported-by: Jeremy Cline <jeremy@jcline.org>
> Cc: Steven Presser <steve@pressers.name>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

So as already discussed switching all devices which define a BOSC0200
device in their ACPI tables to using i2c-multi-instantiate.c will
break the orientation quirks in udev's hwdb since those go by
the modalias and this will change the modalias from acpi:BOSC0200:BOSC0200
to i2c:bmc150_accel

After our last discussion I've been thinking about this and I think the
best solution is something like this in drivers/acpi/scan.c:

#define I2C_MULTI_INST_FORCE      0x01

         static const struct acpi_device_id i2c_multi_instantiate_ids[] = {
                 {"BOSC0200", },
                 {"BSG1160", },
                 {"INT33FE", I2C_MULTI_INST_FORCE},
                 {"INT3515", },
                 {}
         };
	const struct acpi_device_id *id = NULL;

         ...

	/* Instantiate a pdev for the i2c-multi-instantiate drv to bind to? */
	__acpi_match_device(device, i2c_multi_instantiate_ids, NULL, &id, NULL);
	if (id) {
		if (id->driver_data & I2C_MULTI_INST_FORCE)
			return false;

                 count = i2c_multi_inst_count_resources(device);
		if (count > 1)
			return false;
	}

This way we only move to a platform-device + i2c-multi-inst for BOSC0200
devices on devices which actually have multiple accelerometers defined
in the node, avoiding the problem of breaking hwdb for existing devices.

Note arguably the force flag should be set on all existing entries in
the i2c_multi_instantiate_ids, but at least the BSG1160 entry is expected
to always have multiple I2cSerialBus resources and I guess the same may
apply to the INT3515 case

The weird BYT/CHT INT33FE fake battery device is special and we always
need to not create an i2c-client for it, so that we ignore the
clkrate / speed from its bogus first and sometimes only I2cSerialBus resource.

Regards,

Hans







> ---
>   drivers/acpi/scan.c                          | 1 +
>   drivers/iio/accel/bmc150-accel-i2c.c         | 1 -
>   drivers/platform/x86/i2c-multi-instantiate.c | 7 +++++++
>   3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index e9eda5558c1f..e46c877fa9f1 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1539,6 +1539,7 @@ static bool acpi_device_enumeration_by_parent(struct acpi_device *device)
>   	 * which i2c_device_id to use for each resource.
>   	 */
>   	static const struct acpi_device_id i2c_multi_instantiate_ids[] = {
> +		{"BOSC0200", },
>   		{"BSG1160", },
>   		{"INT33FE", },
>   		{"INT3515", },
> diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
> index 8ffc308d5fd0..9d22a4d9d568 100644
> --- a/drivers/iio/accel/bmc150-accel-i2c.c
> +++ b/drivers/iio/accel/bmc150-accel-i2c.c
> @@ -64,7 +64,6 @@ static const struct acpi_device_id bmc150_accel_acpi_match[] = {
>   	{"BMA250E",	bma250e},
>   	{"BMA222E",	bma222e},
>   	{"BMA0280",	bma280},
> -	{"BOSC0200"},
>   	{ },
>   };
>   MODULE_DEVICE_TABLE(acpi, bmc150_accel_acpi_match);
> diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
> index efaf34cbbc7b..c90137d30a98 100644
> --- a/drivers/platform/x86/i2c-multi-instantiate.c
> +++ b/drivers/platform/x86/i2c-multi-instantiate.c
> @@ -151,6 +151,12 @@ static int i2c_multi_inst_remove(struct platform_device *pdev)
>   	return 0;
>   }
>   
> +static const struct i2c_inst_data bosc0200_data[]  = {
> +	{ "bmc150_accel" },
> +	{ "bmc150_accel" },
> +	{}
> +};
> +
>   static const struct i2c_inst_data bsg1160_data[]  = {
>   	{ "bmc150_accel", IRQ_RESOURCE_GPIO, 0 },
>   	{ "bmc150_magn" },
> @@ -171,6 +177,7 @@ static const struct i2c_inst_data int3515_data[]  = {
>    * drivers/acpi/scan.c: acpi_device_enumeration_by_parent().
>    */
>   static const struct acpi_device_id i2c_multi_inst_acpi_ids[] = {
> +	{ "BOSC0200", (unsigned long)bosc0200_data },
>   	{ "BSG1160", (unsigned long)bsg1160_data },
>   	{ "INT3515", (unsigned long)int3515_data },
>   	{ }
> 

Re: [PATCH v1 06/15] i2c: acpi: Assign fwnode for devices created via i2c_acpi_new_device()

[Andy Shevchenko]

On Wed, Nov 21, 2018 at 12:44:27PM +0100, Hans de Goede wrote:
> On 20-11-18 16:59, Andy Shevchenko wrote:
> > i2c_acpi_new_device() doesn't assign fwnode like it's done, for example,
> > in i2c_acpi_register_devices() path.
> > 
> > Assign fwnode in i2c_acpi_new_device() as it's done elsewhere.

> As already mentioned in my reply to the cover letter, a NACK from me for
> this one.
> 
> The problem is that this causes the fwnode to be shared by all the
> "struct device"-s (embedded in the i2c-clients created).
> 
> This in itself is not necessarily a problem, but it does become a
> problem when combined with using device_add_properties as
> intel_cht_int33fe.c does.
> 
> Since the info in the ACPI tables for devices which use this
> driver is pretty terrible, we add a bunch of properties to notify
> the drivers about how the different parts which make up the Type-C
> functionality are tied together.
> 
> device_add_properties creates a new fwnode and then calls
> set_secondary_fwnode. For devices which already have a fwnode
> assigned (which the i2c-clients will have after this patch)
> set_secondary_fwnode sets dev->fwnode->secondary to the fwnode
> which has been newly created to hold the added properties.
> 
> Since all i2c-clients instantiated now share dev->fwnode,
> the result of this is that all i2c-clients created now will
> have the properties of the last i2c-client instantiated for
> an acpi-node which describes multiple clients in a single
> node.
> 
> Also the fwnodes created  for the properties of earlier
> instantiated i2c-clients will be leaked.

Thanks for this very detailed problem report.

> 
> I see 2 possible solutions here:
> 
> 1) Changing this patch so that i2c_acpi_new_device() does:
> 
>         /*
>          * The fwnode can not be shared between instantiated clients when
>          * adding device-properties to the instantiated client.
>          */
> 	if (!info->properties)
> 		info->fwnode = acpi_fwnode_handle(adev);
> 
> 2) Leave whether info->fwnode should be set or not up to the
> caller of i2c_acpi_new_device() (info is already passed in as
> a param). E.g. i2c-multi-instantiate.c could do this (for now,
> maybe in the future we will also want to add properties for
> some HIDs).
> 
> I think that 1. is the best solution and this is all kernel internal,
> so we can always revisit this if necessary.

Heikki told me he has some ideas, but for now I will drop this patch.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 06/15] i2c: acpi: Assign fwnode for devices created via i2c_acpi_new_device()

[Heikki Krogerus]

Hi guys,

On Wed, Nov 21, 2018 at 03:35:49PM +0200, Andy Shevchenko wrote:
> On Wed, Nov 21, 2018 at 12:44:27PM +0100, Hans de Goede wrote:

> > I see 2 possible solutions here:
> > 
> > 1) Changing this patch so that i2c_acpi_new_device() does:
> > 
> >         /*
> >          * The fwnode can not be shared between instantiated clients when
> >          * adding device-properties to the instantiated client.
> >          */
> > 	if (!info->properties)
> > 		info->fwnode = acpi_fwnode_handle(adev);
> > 
> > 2) Leave whether info->fwnode should be set or not up to the
> > caller of i2c_acpi_new_device() (info is already passed in as
> > a param). E.g. i2c-multi-instantiate.c could do this (for now,
> > maybe in the future we will also want to add properties for
> > some HIDs).
> > 
> > I think that 1. is the best solution and this is all kernel internal,
> > so we can always revisit this if necessary.
> 
> Heikki told me he has some ideas, but for now I will drop this patch.

I think the only thing that we can do is to assign the ACPI node as the
secondary node to this kind of mfd "cells". It would most likely
require some changes to the apci code in Linux kernel, probable
nothing major, but no driver needs this for now (I think), so we don't
need to worry about it right now.


thanks,

-- 
heikki

Re: [PATCH v1 09/15] platform/x86: i2c-multi-instantiate: Count I2cSerialBus() resources

[Andy Shevchenko]

On Wed, Nov 21, 2018 at 01:03:35PM +0100, Hans de Goede wrote:
> On 20-11-18 16:59, Andy Shevchenko wrote:

> > -	multi->num_clients = i;
> > +	multi->num_clients = ret;
> >   	for (i = 0; i < multi->num_clients; i++) {
> >   		memset(&board_info, 0, sizeof(board_info));
> 
> Now you end up indexing inst_data with i without checking that inst_data
> holds enough data. Not using all of inst_data in case there are less
> I2cSerialBus resources then inst_data entries is fine. But you need to
> deal with the case when there are more I2cSerialBus resources then inst_data
> entries, either by erroring out or ignoring the extra I2cSerialBus resources.

I think returning an error here is to be on safer side.

> Also it might be better to make the i2c_multi_inst_count_resources() function
> a generic helper under drivers/acpi, so that it can be used in other places,
> see my comments to the BOSC0200 patch.

I would rather to avoid touching BOSC0200 right now and keep that function
inside i2c-multi-instantiate.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 00/15] i2c-multi-instantiate: Adapt for INT3515 and alike

[Andy Shevchenko]

On Wed, Nov 21, 2018 at 01:00:07PM +0100, Hans de Goede wrote:
> On 21-11-18 12:27, Hans de Goede wrote:

> > Patches 1-5, 7-13 and 15 are: >
> > Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Thanks!

> 
> Correction, I also have some small remarks to patch 9:
> 
> [PATCH v1 09/15] platform/x86: i2c-multi-instantiate: Count I2cSerialBus() resources
> 
> Which will need to be addressed before you can add my Reviewed-by.

Will be addressed for v2.

Thanks for review!

-- 
With Best Regards,
Andy Shevchenko