[PATCH v3 00/13] i2c-multi-instantiate: Adapt for INT3515 and alike

[PATCH v3 00/13] 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.

Note, series has been smoke tested (Heikki would do BAT or more)
on Intel Coffee Lake system.

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.

In v3:
- revisit error codes returned by i2c_acpi_new_device() (Hans, Mika)
- add documentation to exported i2c_acpi_get_i2c_resource() (Mika)
- drop Hans' Rb tag in patch 5 due to changes
- append Mika's Ab tag to patch 6

In v2:
- drop patches to copy fwnode and to handle BOSC0200 device
- append check for amount of devices listed in the driver's mapping
- add Rb tag given by Hans

Andy Shevchenko (13):
  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-multi-instantiate: Defer probe when no adapter found
  i2c: acpi: Return error pointers from 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
  iio: inv_mpu6050: Use i2c_acpi_get_i2c_resource() helper

 drivers/acpi/scan.c                          |  1 +
 drivers/i2c/i2c-core-acpi.c                  | 64 +++++++++----
 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 +++
 7 files changed, 174 insertions(+), 59 deletions(-)

--
2.19.2

[PATCH v3 02/13] 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 intel_cht_int33fe driver to support that.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/intel_cht_int33fe.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
index 431151d4e611..3ba139d3bd03 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -169,7 +169,13 @@ static int cht_int33fe_probe(struct platform_device *pdev)
 		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 */
+			ret = -EPROBE_DEFER; /* Wait for i2c-adapter to load */
+		else if (IS_ERR(data->max17047))
+			ret = PTR_ERR(data->max17047);
+		else
+			ret = 0;
+		if (ret)
+			return ret;
 	}
 
 	data->connections[0].endpoint[0] = "port0";
@@ -195,6 +201,12 @@ static int cht_int33fe_probe(struct platform_device *pdev)
 
 	data->fusb302 = i2c_acpi_new_device(dev, 2, &board_info);
 	if (!data->fusb302)
+		ret = -EPROBE_DEFER; /* Wait for the i2c-adapter to load */
+	else if (IS_ERR(data->fusb302))
+		ret = PTR_ERR(data->fusb302);
+	else
+		ret = 0;
+	if (ret)
 		goto out_unregister_max17047;
 
 	memset(&board_info, 0, sizeof(board_info));
@@ -203,6 +215,12 @@ static int cht_int33fe_probe(struct platform_device *pdev)
 
 	data->pi3usb30532 = i2c_acpi_new_device(dev, 3, &board_info);
 	if (!data->pi3usb30532)
+		ret = -EPROBE_DEFER; /* Wait for the i2c-adapter to load */
+	else if (IS_ERR(data->pi3usb30532))
+		ret = PTR_ERR(data->pi3usb30532);
+	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.2

[PATCH v3 03/13] 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>
Reviewed-by: Hans de Goede <hdegoede@redhat.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..bab79bdeebc7 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 (!multi->clients[i])
 			ret = -ENODEV;
+		else if (IS_ERR(multi->clients[i]))
+			ret = PTR_ERR(multi->clients[i]);
+		else
+			ret = 0;
+		if (ret) {
+			dev_err(dev, "Error creating i2c-client, idx %d\n", i);
 			goto error;
 		}
 	}
-- 
2.19.2

[PATCH v3 04/13] platform/x86: i2c-multi-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>
Reviewed-by: Hans de Goede <hdegoede@redhat.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 bab79bdeebc7..0e3a36f3ef64 100644
--- a/drivers/platform/x86/i2c-multi-instantiate.c
+++ b/drivers/platform/x86/i2c-multi-instantiate.c
@@ -73,7 +73,7 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
 		}
 		multi->clients[i] = i2c_acpi_new_device(dev, i, &board_info);
 		if (!multi->clients[i])
-			ret = -ENODEV;
+			ret = -EPROBE_DEFER; /* Wait for i2c-adapter to load */
 		else if (IS_ERR(multi->clients[i]))
 			ret = PTR_ERR(multi->clients[i]);
 		else
-- 
2.19.2

[PATCH v3 05/13] 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 invalid argument supplied.
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                  | 21 ++++++++++++++------
 drivers/platform/x86/i2c-multi-instantiate.c |  2 +-
 drivers/platform/x86/intel_cht_int33fe.c     |  6 +++---
 3 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index 32affd3fa8bd..7e872ceaa14f 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -386,20 +386,22 @@ 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.
+ * Returns a pointer to the new i2c-client, or error pointer in case of failure.
+ * Specifically, -ENODEV is returned if the adapter is not found.
  */
 struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
 				       struct i2c_board_info *info)
 {
 	struct i2c_acpi_lookup lookup;
 	struct i2c_adapter *adapter;
+	struct i2c_client *client;
 	struct acpi_device *adev;
 	LIST_HEAD(resource_list);
 	int ret;
 
 	adev = ACPI_COMPANION(dev);
 	if (!adev)
-		return NULL;
+		return ERR_PTR(-EINVAL);
 
 	memset(&lookup, 0, sizeof(lookup));
 	lookup.info = info;
@@ -408,16 +410,23 @@ 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);
+	if (ret < 0)
+		return ERR_PTR(ret);
+
 	acpi_dev_free_resource_list(&resource_list);
 
-	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)
-		return NULL;
+		return ERR_PTR(-ENODEV);
+
+	client = i2c_new_device(adapter, info);
+	if (!client)
+		return ERR_PTR(-ENODEV);
 
-	return i2c_new_device(adapter, info);
+	return client;
 }
 EXPORT_SYMBOL_GPL(i2c_acpi_new_device);
 
diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
index 0e3a36f3ef64..18928e4ede7f 100644
--- a/drivers/platform/x86/i2c-multi-instantiate.c
+++ b/drivers/platform/x86/i2c-multi-instantiate.c
@@ -72,7 +72,7 @@ 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])
+		if (PTR_ERR(multi->clients[i]) == -ENODEV)
 			ret = -EPROBE_DEFER; /* Wait for i2c-adapter to load */
 		else if (IS_ERR(multi->clients[i]))
 			ret = PTR_ERR(multi->clients[i]);
diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
index 3ba139d3bd03..b8cab17ec596 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -168,7 +168,7 @@ 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)
+		if (PTR_ERR(data->max17047) == -ENODEV)
 			ret = -EPROBE_DEFER; /* Wait for i2c-adapter to load */
 		else if (IS_ERR(data->max17047))
 			ret = PTR_ERR(data->max17047);
@@ -200,7 +200,7 @@ 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 (PTR_ERR(data->fusb302) == -ENODEV)
 		ret = -EPROBE_DEFER; /* Wait for the i2c-adapter to load */
 	else if (IS_ERR(data->fusb302))
 		ret = PTR_ERR(data->fusb302);
@@ -214,7 +214,7 @@ 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 (PTR_ERR(data->pi3usb30532) == -ENODEV)
 		ret = -EPROBE_DEFER; /* Wait for the i2c-adapter to load */
 	else if (IS_ERR(data->pi3usb30532))
 		ret = PTR_ERR(data->pi3usb30532);
-- 
2.19.2

[PATCH v3 06/13] 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>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Mika Westerberg <mika.westerberg@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 7e872ceaa14f..4cebbd280e36 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.2

[PATCH v3 07/13] 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>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/i2c-core-acpi.c | 41 ++++++++++++++++++++++++++-----------
 include/linux/acpi.h        | 11 ++++++++++
 2 files changed, 40 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index 4cebbd280e36..ca707e8319bb 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -45,6 +45,33 @@ struct i2c_acpi_lookup {
 	u32 min_speed;
 };
 
+/**
+ * i2c_acpi_get_i2c_resource - Gets I2cSerialBus resource if type matches
+ * @ares:	ACPI resource
+ * @i2c:	Pointer to I2cSerialBus resource will be returned here
+ *
+ * Checks if the given ACPI resource is of type I2cSerialBus.
+ * In this case, returns a pointer to it to the caller.
+ *
+ * Returns true if resource type is of I2cSerialBus, otherwise false.
+ */
+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 +79,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)
@@ -534,13 +557,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.2

[PATCH v3 08/13] 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>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/i2c-multi-instantiate.c | 41 ++++++++++++++++++--
 1 file changed, 37 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
index 18928e4ede7f..f82f0b36f38d 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,17 +70,19 @@ 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++) {
+	for (i = 0; i < multi->num_clients && inst_data[i].type; 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,
@@ -83,6 +111,11 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
 			goto error;
 		}
 	}
+	if (i < multi->num_clients) {
+		dev_err(dev, "Error finding driver, idx %d\n", i);
+		ret = -ENODEV;
+		goto error;
+	}
 
 	platform_set_drvdata(pdev, multi);
 	return 0;
-- 
2.19.2

[PATCH v3 09/13] 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>
Reviewed-by: Hans de Goede <hdegoede@redhat.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 f82f0b36f38d..efee77a11419 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 (PTR_ERR(multi->clients[i]) == -ENODEV)
@@ -139,9 +148,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.2

[PATCH v3 10/13] 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>
Reviewed-by: Hans de Goede <hdegoede@redhat.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 efee77a11419..a1e6ec12ef73 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.2

[PATCH v3 11/13] 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>
Reviewed-by: Hans de Goede <hdegoede@redhat.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 a1e6ec12ef73..2e4ec09c57b6 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 && inst_data[i].type; 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.2

[PATCH v3 12/13] 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>
Reviewed-by: Hans de Goede <hdegoede@redhat.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 2e4ec09c57b6..f315c755a965 100644
--- a/drivers/platform/x86/i2c-multi-instantiate.c
+++ b/drivers/platform/x86/i2c-multi-instantiate.c
@@ -163,12 +163,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.2

[PATCH v3 13/13] 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>
Reviewed-by: Hans de Goede <hdegoede@redhat.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.2

Re: [PATCH v3 05/13] i2c: acpi: Return error pointers from i2c_acpi_new_device()

[Hans de Goede]

Hi,

On 27-11-18 16:37, Andy Shevchenko wrote:
> 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 invalid argument supplied.
> 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                  | 21 ++++++++++++++------
>   drivers/platform/x86/i2c-multi-instantiate.c |  2 +-
>   drivers/platform/x86/intel_cht_int33fe.c     |  6 +++---
>   3 files changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> index 32affd3fa8bd..7e872ceaa14f 100644
> --- a/drivers/i2c/i2c-core-acpi.c
> +++ b/drivers/i2c/i2c-core-acpi.c
> @@ -386,20 +386,22 @@ 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.
> + * Returns a pointer to the new i2c-client, or error pointer in case of failure.
> + * Specifically, -ENODEV is returned if the adapter is not found.
>    */
>   struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
>   				       struct i2c_board_info *info)
>   {
>   	struct i2c_acpi_lookup lookup;
>   	struct i2c_adapter *adapter;
> +	struct i2c_client *client;
>   	struct acpi_device *adev;
>   	LIST_HEAD(resource_list);
>   	int ret;
>   
>   	adev = ACPI_COMPANION(dev);
>   	if (!adev)
> -		return NULL;
> +		return ERR_PTR(-EINVAL);
>   
>   	memset(&lookup, 0, sizeof(lookup));
>   	lookup.info = info;
> @@ -408,16 +410,23 @@ 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);
> +	if (ret < 0)
> +		return ERR_PTR(ret);
> +
>   	acpi_dev_free_resource_list(&resource_list);
>   
> -	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)
> -		return NULL;
> +		return ERR_PTR(-ENODEV);

Why not simply return -EPROBE_DEFER here (and simplify the callers a lot).

This is the only case where we really want to defer.


> +
> +	client = i2c_new_device(adapter, info);
> +	if (!client)
> +		return ERR_PTR(-ENODEV);

If you look at i2c_new_device, it can fail because it is
out of memory, the i2c slave address is invalid, or
their already is an i2c slave with the same address,
iow if this were to return an ERR_PTR itself, this
would return -ENOMEM, -EINVAL or -EBUSY and never
-EPROBE_DEFER.

Note keeping the -ENODEV here is fine, what I'm trying
to say is that in this case the caller of i2c_acpi_new_device
really should not return PROBE_DEFER, so directly returning
EPROBE_DEFER above, rather then let the caller guess is better.

And e.g. this:

                 multi->clients[i] = i2c_acpi_new_device(dev, i, &board_info);
                 if (IS_ERR(multi->clients[i]))
                         ret = PTR_ERR(multi->clients[i]);
                 else if (!multi->clients[i])
                         ret = -EPROBE_DEFER; /* Wait for i2c-adapter to load */
                 else
                         ret = 0;
                 if (ret) {
                         dev_err(dev, "Error creating i2c-client, idx %d\n", i);
                         goto error;
                 }

Can be simplified to:

                 multi->clients[i] = i2c_acpi_new_device(dev, i, &board_info);
                 if (IS_ERR(multi->clients[i])) {
                         ret = PTR_ERR(multi->clients[i]);
                         dev_err(dev, "Error creating i2c-client, idx %d\n", i);
                         goto error;
                 }

This will also allow you to split out the patches cleaning up the callers
without having a bisect problem (the NULL case will now simply never happen).

Regards,

Hans


>   
> -	return i2c_new_device(adapter, info);
> +	return client;
>   }
>   EXPORT_SYMBOL_GPL(i2c_acpi_new_device);
>   
> diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
> index 0e3a36f3ef64..18928e4ede7f 100644
> --- a/drivers/platform/x86/i2c-multi-instantiate.c
> +++ b/drivers/platform/x86/i2c-multi-instantiate.c
> @@ -72,7 +72,7 @@ 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])
> +		if (PTR_ERR(multi->clients[i]) == -ENODEV)
>   			ret = -EPROBE_DEFER; /* Wait for i2c-adapter to load */
>   		else if (IS_ERR(multi->clients[i]))
>   			ret = PTR_ERR(multi->clients[i]);


> diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
> index 3ba139d3bd03..b8cab17ec596 100644
> --- a/drivers/platform/x86/intel_cht_int33fe.c
> +++ b/drivers/platform/x86/intel_cht_int33fe.c
> @@ -168,7 +168,7 @@ 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)
> +		if (PTR_ERR(data->max17047) == -ENODEV)
>   			ret = -EPROBE_DEFER; /* Wait for i2c-adapter to load */
>   		else if (IS_ERR(data->max17047))
>   			ret = PTR_ERR(data->max17047);
> @@ -200,7 +200,7 @@ 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 (PTR_ERR(data->fusb302) == -ENODEV)
>   		ret = -EPROBE_DEFER; /* Wait for the i2c-adapter to load */
>   	else if (IS_ERR(data->fusb302))
>   		ret = PTR_ERR(data->fusb302);
> @@ -214,7 +214,7 @@ 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 (PTR_ERR(data->pi3usb30532) == -ENODEV)
>   		ret = -EPROBE_DEFER; /* Wait for the i2c-adapter to load */
>   	else if (IS_ERR(data->pi3usb30532))
>   		ret = PTR_ERR(data->pi3usb30532);
> 

Re: [PATCH v3 05/13] i2c: acpi: Return error pointers from i2c_acpi_new_device()

[Hans de Goede]

Hi,

On 27-11-18 17:14, Hans de Goede wrote:
> Hi,
> 
> On 27-11-18 16:37, Andy Shevchenko wrote:
>> 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 invalid argument supplied.
>> 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                  | 21 ++++++++++++++------
>>   drivers/platform/x86/i2c-multi-instantiate.c |  2 +-
>>   drivers/platform/x86/intel_cht_int33fe.c     |  6 +++---
>>   3 files changed, 19 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
>> index 32affd3fa8bd..7e872ceaa14f 100644
>> --- a/drivers/i2c/i2c-core-acpi.c
>> +++ b/drivers/i2c/i2c-core-acpi.c
>> @@ -386,20 +386,22 @@ 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.
>> + * Returns a pointer to the new i2c-client, or error pointer in case of failure.
>> + * Specifically, -ENODEV is returned if the adapter is not found.
>>    */
>>   struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
>>                          struct i2c_board_info *info)
>>   {
>>       struct i2c_acpi_lookup lookup;
>>       struct i2c_adapter *adapter;
>> +    struct i2c_client *client;
>>       struct acpi_device *adev;
>>       LIST_HEAD(resource_list);
>>       int ret;
>>       adev = ACPI_COMPANION(dev);
>>       if (!adev)
>> -        return NULL;
>> +        return ERR_PTR(-EINVAL);
>>       memset(&lookup, 0, sizeof(lookup));
>>       lookup.info = info;
>> @@ -408,16 +410,23 @@ 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);
>> +    if (ret < 0)
>> +        return ERR_PTR(ret);
>> +
>>       acpi_dev_free_resource_list(&resource_list);
>> -    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)
>> -        return NULL;
>> +        return ERR_PTR(-ENODEV);
> 
> Why not simply return -EPROBE_DEFER here (and simplify the callers a lot).
> 
> This is the only case where we really want to defer.
> 
> 
>> +
>> +    client = i2c_new_device(adapter, info);
>> +    if (!client)
>> +        return ERR_PTR(-ENODEV);
> 
> If you look at i2c_new_device, it can fail because it is
> out of memory, the i2c slave address is invalid, or
> their already is an i2c slave with the same address,
> iow if this were to return an ERR_PTR itself, this
> would return -ENOMEM, -EINVAL or -EBUSY and never
> -EPROBE_DEFER.
> 
> Note keeping the -ENODEV here is fine, what I'm trying
> to say is that in this case the caller of i2c_acpi_new_device
> really should not return PROBE_DEFER, so directly returning
> EPROBE_DEFER above, rather then let the caller guess is better.
> 
> And e.g. this:
> 
>                  multi->clients[i] = i2c_acpi_new_device(dev, i, &board_info);
>                  if (IS_ERR(multi->clients[i]))
>                          ret = PTR_ERR(multi->clients[i]);
>                  else if (!multi->clients[i])
>                          ret = -EPROBE_DEFER; /* Wait for i2c-adapter to load */
>                  else
>                          ret = 0;
>                  if (ret) {
>                          dev_err(dev, "Error creating i2c-client, idx %d\n", i);
>                          goto error;
>                  }
> 
> Can be simplified to:
> 
>                  multi->clients[i] = i2c_acpi_new_device(dev, i, &board_info);
>                  if (IS_ERR(multi->clients[i])) {
>                          ret = PTR_ERR(multi->clients[i]);
>                          dev_err(dev, "Error creating i2c-client, idx %d\n", i);
>                          goto error;
>                  }

Correction, that should be:

                 multi->clients[i] = i2c_acpi_new_device(dev, i, &board_info);
                 if (IS_ERR(multi->clients[i])) {
                         ret = PTR_ERR(multi->clients[i]);
                         if (ret != -EPROBE_DEFER)
                                 dev_err(dev, "Error creating i2c-client, idx %d\n", i);
                         goto error;
                 }

We don't want to log an error when deferring.

Regards,

Hans



> 
> This will also allow you to split out the patches cleaning up the callers
> without having a bisect problem (the NULL case will now simply never happen).
> 
> Regards,
> 
> Hans
> 
> 
>> -    return i2c_new_device(adapter, info);
>> +    return client;
>>   }
>>   EXPORT_SYMBOL_GPL(i2c_acpi_new_device);
>> diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
>> index 0e3a36f3ef64..18928e4ede7f 100644
>> --- a/drivers/platform/x86/i2c-multi-instantiate.c
>> +++ b/drivers/platform/x86/i2c-multi-instantiate.c
>> @@ -72,7 +72,7 @@ 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])
>> +        if (PTR_ERR(multi->clients[i]) == -ENODEV)
>>               ret = -EPROBE_DEFER; /* Wait for i2c-adapter to load */
>>           else if (IS_ERR(multi->clients[i]))
>>               ret = PTR_ERR(multi->clients[i]);
> 
> 
>> diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
>> index 3ba139d3bd03..b8cab17ec596 100644
>> --- a/drivers/platform/x86/intel_cht_int33fe.c
>> +++ b/drivers/platform/x86/intel_cht_int33fe.c
>> @@ -168,7 +168,7 @@ 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)
>> +        if (PTR_ERR(data->max17047) == -ENODEV)
>>               ret = -EPROBE_DEFER; /* Wait for i2c-adapter to load */
>>           else if (IS_ERR(data->max17047))
>>               ret = PTR_ERR(data->max17047);
>> @@ -200,7 +200,7 @@ 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 (PTR_ERR(data->fusb302) == -ENODEV)
>>           ret = -EPROBE_DEFER; /* Wait for the i2c-adapter to load */
>>       else if (IS_ERR(data->fusb302))
>>           ret = PTR_ERR(data->fusb302);
>> @@ -214,7 +214,7 @@ 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 (PTR_ERR(data->pi3usb30532) == -ENODEV)
>>           ret = -EPROBE_DEFER; /* Wait for the i2c-adapter to load */
>>       else if (IS_ERR(data->pi3usb30532))
>>           ret = PTR_ERR(data->pi3usb30532);
>>

Re: [PATCH v3 05/13] i2c: acpi: Return error pointers from i2c_acpi_new_device()

[Andy Shevchenko]

On Tue, Nov 27, 2018 at 05:14:06PM +0100, Hans de Goede wrote:
> On 27-11-18 16:37, Andy Shevchenko wrote:
> > 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 invalid argument supplied.
> > To achieve this, return error pointer in some cases.

> >   	acpi_dev_free_resource_list(&resource_list);
> > -	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)
> > -		return NULL;
> > +		return ERR_PTR(-ENODEV);

> Why not simply return -EPROBE_DEFER here (and simplify the callers a lot).

> This is the only case where we really want to defer.

> > +	client = i2c_new_device(adapter, info);
> > +	if (!client)
> > +		return ERR_PTR(-ENODEV);
> 
> If you look at i2c_new_device, it can fail because it is
> out of memory, the i2c slave address is invalid, or
> their already is an i2c slave with the same address,
> iow if this were to return an ERR_PTR itself, this
> would return -ENOMEM, -EINVAL or -EBUSY and never
> -EPROBE_DEFER.

It would change the behaviour.

In any case, it's only two users and both written by you, so, just to be sure
you aware of this change and bless it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 05/13] i2c: acpi: Return error pointers from i2c_acpi_new_device()

[Hans de Goede]

Hi,

On 27-11-18 20:27, Andy Shevchenko wrote:
> On Tue, Nov 27, 2018 at 05:14:06PM +0100, Hans de Goede wrote:
>> On 27-11-18 16:37, Andy Shevchenko wrote:
>>> 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 invalid argument supplied.
>>> To achieve this, return error pointer in some cases.
> 
>>>    	acpi_dev_free_resource_list(&resource_list);
>>> -	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)
>>> -		return NULL;
>>> +		return ERR_PTR(-ENODEV);
> 
>> Why not simply return -EPROBE_DEFER here (and simplify the callers a lot).
> 
>> This is the only case where we really want to defer.
> 
>>> +	client = i2c_new_device(adapter, info);
>>> +	if (!client)
>>> +		return ERR_PTR(-ENODEV);
>>
>> If you look at i2c_new_device, it can fail because it is
>> out of memory, the i2c slave address is invalid, or
>> their already is an i2c slave with the same address,
>> iow if this were to return an ERR_PTR itself, this
>> would return -ENOMEM, -EINVAL or -EBUSY and never
>> -EPROBE_DEFER.
> 
> It would change the behaviour.

Yes it would change behaviour, for the better, all the errors
from i2c_new_device() (*) will not go away when we retry later,
so responding with probe-deferring to them is not useful.

> In any case, it's only two users and both written by you, so, just to be sure
> you aware of this change and bless it.

I'm aware and you've my ack for this change.

Regards,

Hans


*) with exception of -ENOMEM which should never happen