Skip to contentSkip to conversation

i2c-multi-instantiate: Adapt for INT3515 and alike

PATCH Superseded Andy Shevchenko 3 participants 23 messages Nov 26, 2018 – Nov 27, 2018

Subsystems (9)

v2 of 5 Superseded Latest is v5 →

lore
Share & tools
lore
#0 lore raw

v2 cover i2c-multi-instantiate: Adapt for INT3515 and alike

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 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-mutli-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                  | 42 +++++----
 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, 155 insertions(+), 56 deletions(-)

--
2.19.1

#1 lore raw

PATCH v2 1/13 platform/x86: intel_cht_int33fe: Remove duplicate NULL check

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

---
 drivers/platform/x86/intel_cht_int33fe.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
drivers/platform/x86/intel_cht_int33fe.c +2 -4
index 464fe93657b5..431151d4e611 100644
@@ -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);
 
Signature · 1 line
2.19.1
#2 lore raw

PATCH v2 2/13 platform/x86: intel_cht_int33fe: Accept errors of i2c_acpi_new_device()

In reply to Andy Shevchenko

In the future i2c_acpi_new_device() will return error pointer in some cases. Prepare intel_cht_int33fe driver to support that.

---
 drivers/platform/x86/intel_cht_int33fe.c | 28 +++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)
drivers/platform/x86/intel_cht_int33fe.c +23 -5
index 431151d4e611..367d6e304ade 100644
@@ -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)
Signature · 1 line
2.19.1
#3 lore raw

PATCH v2 3/13 platform/x86: i2c-multi-instantiate: Accept errors of i2c_acpi_new_device()

In reply to Andy Shevchenko

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

---
 drivers/platform/x86/i2c-multi-instantiate.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
drivers/platform/x86/i2c-multi-instantiate.c +7 -2
index 5456581b473c..e3345da82c84 100644
@@ -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;
 		}
 	}
Signature · 1 line
2.19.1
#4 lore raw

PATCH v2 4/13 platform/x86: i2c-mutli-instantiate: Defer probe when no adapter found

In reply to 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.

---
 drivers/platform/x86/i2c-multi-instantiate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/platform/x86/i2c-multi-instantiate.c +1 -1
index e3345da82c84..16a0eabe1e31 100644
@@ -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) {
Signature · 1 line
2.19.1
#5 lore raw

PATCH v2 5/13 i2c: acpi: Return error pointers from i2c_acpi_new_device()

In reply to 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.

---
 drivers/i2c/i2c-core-acpi.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/i2c/i2c-core-acpi.c +6 -3
index 32affd3fa8bd..af4b5bd5d973 100644
@@ -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)
Signature · 1 line
2.19.1
#6 lore raw

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

Quoted 17 lines · 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.

---
 drivers/i2c/i2c-core-acpi.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/i2c/i2c-core-acpi.c +0 -0
index 32affd3fa8bd..af4b5bd5d973 100644

drivers/i2c/i2c-core-acpi.c struct notifier_block i2c_acpi_notifier = { · line 390

Quoted 4 lines
drivers/i2c/i2c-core-acpi.c +1 -0
@@ -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.

I would rather make it return error pointer always. Then the caller can just check for IS_ERR() and not need to deal with the possible NULL. It is also more consistent that way than saying "some cases might return an error pointer" (but some cases you get NULL or even pointer to the created object) ;-)

#7 lore raw

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

Hi,

Quoted 13 lines · Mika Westerberg
Quoted 12 lines · 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.

---
  drivers/i2c/i2c-core-acpi.c | 9 ++++++---
  1 file changed, 6 insertions(+), 3 deletions(-)
Quoted 15 lines

drivers/i2c/i2c-core-acpi.c struct notifier_block i2c_acpi_notifier = { · line 390

Quoted 9 lines
drivers/i2c/i2c-core-acpi.c +1 -0
index 32affd3fa8bd..af4b5bd5d973 100644
@@ -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.

I would rather make it return error pointer always. Then the caller can just check for IS_ERR() and not need to deal with the possible NULL. It is also more consistent that way than saying "some cases might return an error pointer" (but some cases you get NULL or even pointer to the created object) ;-)

Good one, that will allow for a nice cleanup of the callers, we can make i2c_acpi_new_device return -EPROBE_DEFER when the i2c_acpi_find_adapter_by_handle() call fails, which is exactly the case when we want to defer.

One problem is that i2c_new_device() currently simply returns NULL on all errors. Andy, you could take a look how much work it is to make that return an ERR_PTR too, or just check its return value and return ERR_PTR(-ENXIO) if it fails for now...

Regards,

Hans

#8 lore raw

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

Quoted 4 lines · Hans de Goede

One problem is that i2c_new_device() currently simply returns NULL on all errors. Andy, you could take a look how much work it is to make that return an ERR_PTR too, or just check its return value and return ERR_PTR(-ENXIO) if it fails for now...

I would use -ENODEV here and -EINVAL in case there is no ACPI companion :)

#9 lore raw

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

Quoted 7 lines · Mika Westerberg
Quoted 4 lines · Hans de Goede

One problem is that i2c_new_device() currently simply returns NULL on all errors. Andy, you could take a look how much work it is to make that return an ERR_PTR too, or just check its return value and return ERR_PTR(-ENXIO) if it fails for now...

I would use -ENODEV here and -EINVAL in case there is no ACPI companion :)

Sounds more traditional than ENXIO.
I would go the way Mika proposed if there is no objection.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#10 lore raw

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

Hi,

Quoted 11 lines · Andy Shevchenko
Quoted 7 lines · Mika Westerberg
Quoted 4 lines · Hans de Goede

One problem is that i2c_new_device() currently simply returns NULL on all errors. Andy, you could take a look how much work it is to make that return an ERR_PTR too, or just check its return value and return ERR_PTR(-ENXIO) if it fails for now...

I would use -ENODEV here and -EINVAL in case there is no ACPI companion :)

Sounds more traditional than ENXIO.
I would go the way Mika proposed if there is no objection.

Works for me, go for it.

Regards,

Han

#11 lore raw

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

Quoted 8 lines · Hans de Goede
Quoted 7 lines · Andy Shevchenko
Quoted 6 lines · Mika Westerberg
Quoted 4 lines · Hans de Goede

One problem is that i2c_new_device() currently simply returns NULL on all errors. Andy, you could take a look how much work it is to make that return an ERR_PTR too, or just check its return value and return ERR_PTR(-ENXIO) if it fails for now...

Quoted 6 lines
Quoted 4 lines
Quoted 1 line

I would use -ENODEV here and -EINVAL in case there is no ACPI companion :)

Sounds more traditional than ENXIO.
I would go the way Mika proposed if there is no objection.

Works for me, go for it.

Just sent a new version, but dropped your Rb in that very patch. Please, check if everything is okay.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#12 lore raw

PATCH v2 6/13 i2c: acpi: Use ACPI_FAILURE instead of !ACPI_SUCCESS

In reply to Andy Shevchenko

Convert to use ACPI_FAILURE instead of !ACPI_SUCCESS.

---
 drivers/i2c/i2c-core-acpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/i2c-core-acpi.c +1 -1
index af4b5bd5d973..287a5e4b3d30 100644
@@ -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;
Signature · 1 line
2.19.1
#14 lore raw

PATCH v2 7/13 i2c: acpi: Introduce i2c_acpi_get_i2c_resource() helper

In reply to Andy Shevchenko

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

No functional change intended.

---
 drivers/i2c/i2c-core-acpi.c | 31 +++++++++++++++++++------------
 include/linux/acpi.h        | 11 +++++++++++
 2 files changed, 30 insertions(+), 12 deletions(-)
drivers/i2c/i2c-core-acpi.c +19 -12
index 287a5e4b3d30..b43f535d264b 100644
@@ -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)
@@ -528,13 +541,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;
 	}
include/linux/acpi.h +11 -0
index ed80f147bd50..6afc6e3c4c5c 100644
@@ -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
Signature · 1 line
2.19.1
#15 lore raw

Re: [PATCH v2 07/13] i2c: acpi: Introduce i2c_acpi_get_i2c_resource() helper

Quoted 17 lines · Andy Shevchenko

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

No functional change intended.

---
 drivers/i2c/i2c-core-acpi.c | 31 +++++++++++++++++++------------
 include/linux/acpi.h        | 11 +++++++++++
 2 files changed, 30 insertions(+), 12 deletions(-)
drivers/i2c/i2c-core-acpi.c +0 -0
index 287a5e4b3d30..b43f535d264b 100644

drivers/i2c/i2c-core-acpi.c struct i2c_acpi_lookup { · line 47

Quoted 3 lines
drivers/i2c/i2c-core-acpi.c +0 -0
@@ -45,6 +45,23 @@ struct i2c_acpi_lookup {
 	u32 min_speed;
 };
 

Since this is exported, you may want to add kernel-doc here.

Quoted 55 lines
drivers/i2c/i2c-core-acpi.c +19 -12
+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)
@@ -528,13 +541,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;
 	}
include/linux/acpi.h +0 -0
index ed80f147bd50..6afc6e3c4c5c 100644
@@ -1054,6 +1054,17 @@ static inline int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
 }
 #endif

include/linux/acpi.h static inline int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index) · line 1065

Quoted 10 lines
include/linux/acpi.h +9 -0
 
+#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;
+}

I think this belongs to include/linux/i2c.h where we have i2c_acpi_find_bus_speed() and friends.

#16 lore raw

Re: [PATCH v2 07/13] i2c: acpi: Introduce i2c_acpi_get_i2c_resource() helper

Quoted 5 lines · Mika Westerberg
Quoted 2 lines · Andy Shevchenko

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

Since this is exported, you may want to add kernel-doc here.

OK.

Quoted 5 lines
Quoted 5 lines

+++ b/include/linux/acpi.h

(quoted diff) +0 -0
@@ -1054,6 +1054,17 @@ static inline int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
 }
 #endif
 
Quoted 12 lines

include/linux/acpi.h static inline int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index) · line 1065

Quoted 9 lines
include/linux/acpi.h +9 -0
+#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;
+}

I think this belongs to include/linux/i2c.h where we have i2c_acpi_find_bus_speed() and friends.

I don't think so. It operates on top of data structures defined solely under ACPI umbrella. If I move them to i2c.h it would look inconsistent.

Perhaps you would like to have different namespace for it (like acpi_i2c_ ?).

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#17 lore raw

PATCH v2 8/13 platform/x86: i2c-multi-instantiate: Count I2cSerialBus() resources

In reply to 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.

---
 drivers/platform/x86/i2c-multi-instantiate.c | 41 ++++++++++++++++++--
 1 file changed, 37 insertions(+), 4 deletions(-)
drivers/platform/x86/i2c-multi-instantiate.c +37 -4
index 16a0eabe1e31..2a405867bbc6 100644
@@ -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;
Signature · 1 line
2.19.1
#18 lore raw

PATCH v2 9/13 platform/x86: i2c-multi-instantiate: Distinguish IRQ resource type

In reply to Andy Shevchenko

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

---
 drivers/platform/x86/i2c-multi-instantiate.c | 27 +++++++++++++-------
 1 file changed, 18 insertions(+), 9 deletions(-)
drivers/platform/x86/i2c-multi-instantiate.c +18 -9
index 2a405867bbc6..1faecc99e0a6 100644
@@ -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]))
@@ -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" },
 	{}
 };
 
Signature · 1 line
2.19.1
#19 lore raw

PATCH v2 10/13 platform/x86: i2c-multi-instantiate: Introduce IOAPIC IRQ support

In reply to 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.

---
 drivers/platform/x86/i2c-multi-instantiate.c | 9 +++++++++
 1 file changed, 9 insertions(+)
drivers/platform/x86/i2c-multi-instantiate.c +9 -0
index 1faecc99e0a6..2110f2e4aa1a 100644
@@ -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;
Signature · 1 line
2.19.1
#20 lore raw

PATCH v2 11/13 platform/x86: i2c-multi-instantiate: Allow to have same slaves

In reply to 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.

---
 drivers/platform/x86/i2c-multi-instantiate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/platform/x86/i2c-multi-instantiate.c +2 -2
index 2110f2e4aa1a..d184833bfd05 100644
@@ -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:
Signature · 1 line
2.19.1
#21 lore raw

PATCH v2 12/13 ACPI / scan: Create platform device for INT3515 ACPI nodes

In reply to 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.

---
 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(-)
drivers/acpi/scan.c +1 -0
index bd1c59fb0e17..e9eda5558c1f 100644
@@ -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", },
 		{}
 	};
 
drivers/platform/x86/i2c-multi-instantiate.c +9 -0
index d184833bfd05..ab50dda6062a 100644
@@ -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);
drivers/usb/typec/tps6598x.c +4 -4
index c84c8c189e90..1c0033ad8738 100644
@@ -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);
 
Signature · 1 line
2.19.1
#22 lore raw

PATCH v2 13/13 iio: inv_mpu6050: Use i2c_acpi_get_i2c_resource() helper

In reply to Andy Shevchenko

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

---
 drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)
drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c +6 -10
index d78a10403bac..a961b5a06fe6 100644
@@ -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 */
Signature · 1 line
2.19.1