[PATCH v5 0/4] i2c-multi-instantiate pseudo driver

[PATCH v5 0/4] i2c-multi-instantiate pseudo driver

[Hans de Goede]

Hi All,

Here is v5 of my patch-series for dealing with multiple independent i2c
devices being described in a single ACPI fwnode.

Changes in v5:
-s/no_clients/num_clients/
-Change patch 4 Subject prefix to platform/x86

Changes in v4:
-Rewrite commit message of first patch as this is NOT a bug fix for
 acpi_is_indirect_io_slave() as I thought at first, it is still necessary
 as a preperation patch for the rest of the series
-MAINTAINER and Kconfig entry fixes
-Some minor coding style fixes

Changes in v3:
-Drop the patch adding the I2C_CLIENT_IGNORE_BUSY flag/hack
-Add patches to make ACPI fwnode-s with affected acpi_ids get enumerated as
 platform device instead of i2c-client
-Change the i2c-multi-instantiate driver to a platform driver

Changes in v2:
-Rebase on 4.18-rc2

Changes in v1:
-Second attempt at dealing with this using a new design where a separate
 driver (which can be build as a module) is used to instantiate the
 i2c-clients instead of modifying the i2c core
-Add an I2C_CLIENT_IGNORE_BUSY flag to make it possible for this driver to bind
 to the client instantiated for the first i2c resource and still create a new
 client using the i2c_client_id for the actual driver for the first resource,
 resulting in 2 clients at the same address (of which one is inactive)

About merging this series, the platform-driver and ACPI patches can be
merged independently of each other, they platform driver relies on the ACPI
changes to gets its platform device instantiated, but if those changes are
not in place yet, nothing bad happens.

So if everyone is happy with this series I suggest that each part gets
merged separately through the relevant trees.

Regards,

Hans

[PATCH v5 1/4] ACPI / scan: Initialize status to ACPI_STA_DEFAULT

[Hans de Goede]

Since commit 63347db0affa ("ACPI / scan: Use acpi_bus_get_status() to
initialize ACPI_TYPE_DEVICE devs") the status field of normal acpi_devices
gets set to 0 by acpi_bus_type_and_status() and filled with its actual
value later when acpi_add_single_object() calls acpi_bus_get_status().

This means that any acpi_match_device_ids() calls in between will always
fail with -ENOENT.

We already have a workaround for this, which temporary forces status to
ACPI_STA_DEFAULT in drivers/acpi/x86/utils.c: acpi_device_always_present()
and the next commit in this series adds another acpi_match_device_ids()
call between status being initialized as 0 and the acpi_bus_get_status()
call.

Rather then adding another workaround, this commit makes
acpi_bus_type_and_status() initialize status to ACPI_STA_DEFAULT, this is
safe to do as the only code looking at status between the initialization
and the acpi_bus_get_status() call is those acpi_match_device_ids() calls.

Note this does mean that we need to (re)set status to 0 in case the
acpi_bus_get_status() call fails.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v3:
-New patch in v3 of this patch-set

Changes in v4:
-This is not a fix for acpi_is_indirect_io_slave() as I thought at first,
 acpi_is_indirect_io_slave() calls acpi_match_device_ids() on its parent
 device, where status is already set properly. Rewrite the commit message
 accordingly.
---
 drivers/acpi/scan.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 970dd87d347c..6799d00dd790 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1612,7 +1612,8 @@ static int acpi_add_single_object(struct acpi_device **child,
 	 * Note this must be done before the get power-/wakeup_dev-flags calls.
 	 */
 	if (type == ACPI_BUS_TYPE_DEVICE)
-		acpi_bus_get_status(device);
+		if (acpi_bus_get_status(device) < 0)
+			acpi_set_device_status(device, 0);
 
 	acpi_bus_get_power_flags(device);
 	acpi_bus_get_wakeup_device_flags(device);
@@ -1690,7 +1691,7 @@ static int acpi_bus_type_and_status(acpi_handle handle, int *type,
 		 * acpi_add_single_object updates this once we've an acpi_device
 		 * so that acpi_bus_get_status' quirk handling can be used.
 		 */
-		*sta = 0;
+		*sta = ACPI_STA_DEFAULT;
 		break;
 	case ACPI_TYPE_PROCESSOR:
 		*type = ACPI_BUS_TYPE_PROCESSOR;
-- 
2.18.0

[PATCH v5 2/4] ACPI / scan: Create platform device for fwnodes with multiple i2c devices

[Hans de Goede]

Some devices have multiple I2cSerialBus resources and for things to work
an i2c-client must be instantiated for each, each with its own
i2c_device_id.

Normally we only instantiate an i2c-client for the first resource, using
the ACPI HID as id.

This commit adds a list of HIDs of devices, which need multiple i2c-clients
instantiated from a single fwnode, to acpi_device_enumeration_by_parent and
makes acpi_device_enumeration_by_parent return false for these devices so
that a platform device will be instantiated.

This allows the drivers/platform/x86/i2c-multi-instantiate.c driver, which
knows which i2c_device_id to use for each resource, to bind to the fwnode
and initiate an i2c-client for each resource.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v3:
-New patch in v3 of this patch-set

Changes in v4:
-Some small code-style fixes
---
 drivers/acpi/scan.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 6799d00dd790..b51913372e21 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1540,6 +1540,18 @@ static bool acpi_device_enumeration_by_parent(struct acpi_device *device)
 {
 	struct list_head resource_list;
 	bool is_serial_bus_slave = false;
+	/*
+	 * These devices have multiple I2cSerialBus resources and an i2c-client
+	 * must be instantiated for each, each with its own i2c_device_id.
+	 * Normally we only instantiate an i2c-client for the first resource,
+	 * using the ACPI HID as id. These special cases are handled by the
+	 * drivers/platform/x86/i2c-multi-instantiate.c driver, which knows
+	 * which i2c_device_id to use for each resource.
+	 */
+	static const struct acpi_device_id i2c_multi_instantiate_ids[] = {
+		{"BSG1160", },
+		{}
+	};
 
 	if (acpi_is_indirect_io_slave(device))
 		return true;
@@ -1551,6 +1563,10 @@ static bool acpi_device_enumeration_by_parent(struct acpi_device *device)
 	     fwnode_property_present(&device->fwnode, "baud")))
 		return true;
 
+	/* Instantiate a pdev for the i2c-multi-instantiate drv to bind to */
+	if (!acpi_match_device_ids(device, i2c_multi_instantiate_ids))
+		return false;
+
 	INIT_LIST_HEAD(&resource_list);
 	acpi_dev_get_resources(device, &resource_list,
 			       acpi_check_serial_bus_slave,
-- 
2.18.0

[PATCH v5 3/4] ACPI / x86-utils: Remove status workaround from acpi_device_always_present()

[Hans de Goede]

Now that we init the status field to ACPI_STA_DEFAULT rather then to 0,
the workaround for acpi_match_device_ids() always returning -ENOENT when
status is 0 is no longer needed.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v3:
-New patch in v3 of this patch-set
---
 drivers/acpi/x86/utils.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/acpi/x86/utils.c b/drivers/acpi/x86/utils.c
index ec5b0f190231..a4fb97d64b3b 100644
--- a/drivers/acpi/x86/utils.c
+++ b/drivers/acpi/x86/utils.c
@@ -103,13 +103,9 @@ static const struct always_present_id always_present_ids[] = {
 
 bool acpi_device_always_present(struct acpi_device *adev)
 {
-	u32 *status = (u32 *)&adev->status;
-	u32 old_status = *status;
 	bool ret = false;
 	unsigned int i;
 
-	/* acpi_match_device_ids checks status, so set it to default */
-	*status = ACPI_STA_DEFAULT;
 	for (i = 0; i < ARRAY_SIZE(always_present_ids); i++) {
 		if (acpi_match_device_ids(adev, always_present_ids[i].hid))
 			continue;
@@ -125,15 +121,9 @@ bool acpi_device_always_present(struct acpi_device *adev)
 		    !dmi_check_system(always_present_ids[i].dmi_ids))
 			continue;
 
-		if (old_status != ACPI_STA_DEFAULT) /* Log only once */
-			dev_info(&adev->dev,
-				 "Device [%s] is in always present list\n",
-				 adev->pnp.bus_id);
-
 		ret = true;
 		break;
 	}
-	*status = old_status;
 
 	return ret;
 }
-- 
2.18.0

[PATCH v5 4/4] platform/x86: Add ACPI i2c-multi-instantiate pseudo driver

[Hans de Goede]

On systems with ACPI instantiated i2c-clients, normally there is 1 fw_node
per i2c-device and that fw-node contains 1 I2cSerialBus resource for that 1
i2c-device.

But in some rare cases the manufacturer has decided to describe multiple
i2c-devices in a single ACPI fwnode with multiple I2cSerialBus resources.

An earlier attempt to fix this in the i2c-core resulted in a lot of extra
code to support this corner-case.

This commit introduces a new i2c-multi-instantiate driver which fixes this
in a different way. This new driver can be built as a module which will
only loaded on affected systems.

This driver will instantiate a new i2c-client per I2cSerialBus resource,
using the driver_data from the acpi_device_id it is binding to to tell it
which chip-type (and optional irq-resource) to use when instantiating.

Note this driver depends on a platform device being instantiated for the
ACPI fwnode, see the i2c_multi_instantiate_ids list of ACPI device-ids in
drivers/acpi/scan.c: acpi_device_enumeration_by_parent().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Rebase on top of 4.18-rc2

Changes in v3:
-Change from an i2c-driver using a hack to allow having multiple i2c clients
 at the same address to a platform-driver

Changes in v4:
-Tweak MAINTAINERS entry a bit

Changes in v5:
-s/no_clients/num_clients/
-Change patch Subject prefix to platform/x86
---
 MAINTAINERS                                  |   6 +
 drivers/platform/x86/Kconfig                 |  11 ++
 drivers/platform/x86/Makefile                |   1 +
 drivers/platform/x86/i2c-multi-instantiate.c | 131 +++++++++++++++++++
 4 files changed, 149 insertions(+)
 create mode 100644 drivers/platform/x86/i2c-multi-instantiate.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 9b377508f24f..dbe7836e4f6b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -367,6 +367,12 @@ L:	linux-acpi@vger.kernel.org
 S:	Maintained
 F:	drivers/acpi/arm64
 
+ACPI I2C MULTI INSTANTIATE DRIVER
+M:	Hans de Goede <hdegoede@redhat.com>
+L:	platform-driver-x86@vger.kernel.org
+S:	Maintained
+F:	drivers/platform/x86/i2c-multi-instantiate.c
+
 ACPI PMIC DRIVERS
 M:	"Rafael J. Wysocki" <rjw@rjwysocki.net>
 M:	Len Brown <lenb@kernel.org>
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 85a93453237c..64c82592d4b6 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1219,6 +1219,17 @@ config INTEL_CHTDC_TI_PWRBTN
 	  To compile this driver as a module, choose M here: the module
 	  will be called intel_chtdc_ti_pwrbtn.
 
+config I2C_MULTI_INSTANTIATE
+	tristate "I2C multi instantiate pseudo device driver"
+	depends on I2C && ACPI
+	help
+	  Some ACPI-based systems list multiple i2c-devices in a single ACPI
+	  firmware-node. This driver will instantiate separate i2c-clients
+	  for each device in the firmware-node.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called i2c-multi-instantiate.
+
 endif # X86_PLATFORM_DEVICES
 
 config PMC_ATOM
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 8d9477114fb5..e6d1becf81ce 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -91,3 +91,4 @@ obj-$(CONFIG_PMC_ATOM)		+= pmc_atom.o
 obj-$(CONFIG_MLX_PLATFORM)	+= mlx-platform.o
 obj-$(CONFIG_INTEL_TURBO_MAX_3) += intel_turbo_max_3.o
 obj-$(CONFIG_INTEL_CHTDC_TI_PWRBTN)	+= intel_chtdc_ti_pwrbtn.o
+obj-$(CONFIG_I2C_MULTI_INSTANTIATE)	+= i2c-multi-instantiate.o
diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
new file mode 100644
index 000000000000..75c00630ecde
--- /dev/null
+++ b/drivers/platform/x86/i2c-multi-instantiate.c
@@ -0,0 +1,131 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * I2C multi-instantiate driver, pseudo driver to instantiate multiple
+ * i2c-clients from a single fwnode.
+ *
+ * Copyright 2018 Hans de Goede <hdegoede@redhat.com>
+ */
+
+#include <linux/acpi.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+struct i2c_inst_data {
+	const char *type;
+	int irq_idx;
+};
+
+struct i2c_multi_inst_data {
+	int num_clients;
+	struct i2c_client *clients[0];
+};
+
+static int i2c_multi_inst_probe(struct platform_device *pdev)
+{
+	struct i2c_multi_inst_data *multi;
+	const struct acpi_device_id *match;
+	const struct i2c_inst_data *inst_data;
+	struct i2c_board_info board_info = {};
+	struct device *dev = &pdev->dev;
+	struct acpi_device *adev;
+	char name[32];
+	int i, ret;
+
+	match = acpi_match_device(dev->driver->acpi_match_table, dev);
+	if (!match) {
+		dev_err(dev, "Error ACPI match data is missing\n");
+		return -ENODEV;
+	}
+	inst_data = (const struct i2c_inst_data *)match->driver_data;
+
+	adev = ACPI_COMPANION(dev);
+
+	/* Count number of clients to instantiate */
+	for (i = 0; inst_data[i].type; i++) {}
+
+	multi = devm_kmalloc(dev,
+			offsetof(struct i2c_multi_inst_data, clients[i]),
+			GFP_KERNEL);
+	if (!multi)
+		return -ENOMEM;
+
+	multi->num_clients = i;
+
+	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);
+		board_info.dev_name = name;
+		board_info.irq = 0;
+		if (inst_data[i].irq_idx != -1) {
+			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].irq_idx, ret);
+				goto error;
+			}
+			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);
+			ret = -ENODEV;
+			goto error;
+		}
+	}
+
+	platform_set_drvdata(pdev, multi);
+	return 0;
+
+error:
+	while (--i >= 0)
+		i2c_unregister_device(multi->clients[i]);
+
+	return ret;
+}
+
+static int i2c_multi_inst_remove(struct platform_device *pdev)
+{
+	struct i2c_multi_inst_data *multi = platform_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < multi->num_clients; i++)
+		i2c_unregister_device(multi->clients[i]);
+
+	return 0;
+}
+
+static const struct i2c_inst_data bsg1160_data[]  = {
+	{ "bmc150_accel", 0 },
+	{ "bmc150_magn", -1 },
+	{ "bmg160", -1 },
+	{}
+};
+
+/*
+ * 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 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, i2c_multi_inst_acpi_ids);
+
+static struct platform_driver i2c_multi_inst_driver = {
+	.driver	= {
+		.name = "I2C multi instantiate pseudo device driver",
+		.acpi_match_table = ACPI_PTR(i2c_multi_inst_acpi_ids),
+	},
+	.probe = i2c_multi_inst_probe,
+	.remove = i2c_multi_inst_remove,
+};
+module_platform_driver(i2c_multi_inst_driver);
+
+MODULE_DESCRIPTION("I2C multi instantiate pseudo device driver");
+MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
+MODULE_LICENSE("GPL");
-- 
2.18.0

Re: [PATCH v5 1/4] ACPI / scan: Initialize status to ACPI_STA_DEFAULT

[Rafael J. Wysocki]

On Thu, Aug 9, 2018 at 11:15 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Since commit 63347db0affa ("ACPI / scan: Use acpi_bus_get_status() to
> initialize ACPI_TYPE_DEVICE devs") the status field of normal acpi_devices
> gets set to 0 by acpi_bus_type_and_status() and filled with its actual
> value later when acpi_add_single_object() calls acpi_bus_get_status().
>
> This means that any acpi_match_device_ids() calls in between will always
> fail with -ENOENT.
>
> We already have a workaround for this, which temporary forces status to
> ACPI_STA_DEFAULT in drivers/acpi/x86/utils.c: acpi_device_always_present()
> and the next commit in this series adds another acpi_match_device_ids()
> call between status being initialized as 0 and the acpi_bus_get_status()
> call.
>
> Rather then adding another workaround, this commit makes
> acpi_bus_type_and_status() initialize status to ACPI_STA_DEFAULT, this is
> safe to do as the only code looking at status between the initialization
> and the acpi_bus_get_status() call is those acpi_match_device_ids() calls.
>
> Note this does mean that we need to (re)set status to 0 in case the
> acpi_bus_get_status() call fails.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v3:
> -New patch in v3 of this patch-set
>
> Changes in v4:
> -This is not a fix for acpi_is_indirect_io_slave() as I thought at first,
>  acpi_is_indirect_io_slave() calls acpi_match_device_ids() on its parent
>  device, where status is already set properly. Rewrite the commit message
>  accordingly.

I've applied the v4 of this patch and I don't think there are any
changes from it here.

As for the rest of the series I'll wait from comments from Wolfram and
the other reviewers.

Thanks,
Rafael

Re: [PATCH v5 1/4] ACPI / scan: Initialize status to ACPI_STA_DEFAULT

[Hans de Goede]

Hi,

On 09-08-18 11:35, Rafael J. Wysocki wrote:
> On Thu, Aug 9, 2018 at 11:15 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Since commit 63347db0affa ("ACPI / scan: Use acpi_bus_get_status() to
>> initialize ACPI_TYPE_DEVICE devs") the status field of normal acpi_devices
>> gets set to 0 by acpi_bus_type_and_status() and filled with its actual
>> value later when acpi_add_single_object() calls acpi_bus_get_status().
>>
>> This means that any acpi_match_device_ids() calls in between will always
>> fail with -ENOENT.
>>
>> We already have a workaround for this, which temporary forces status to
>> ACPI_STA_DEFAULT in drivers/acpi/x86/utils.c: acpi_device_always_present()
>> and the next commit in this series adds another acpi_match_device_ids()
>> call between status being initialized as 0 and the acpi_bus_get_status()
>> call.
>>
>> Rather then adding another workaround, this commit makes
>> acpi_bus_type_and_status() initialize status to ACPI_STA_DEFAULT, this is
>> safe to do as the only code looking at status between the initialization
>> and the acpi_bus_get_status() call is those acpi_match_device_ids() calls.
>>
>> Note this does mean that we need to (re)set status to 0 in case the
>> acpi_bus_get_status() call fails.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v3:
>> -New patch in v3 of this patch-set
>>
>> Changes in v4:
>> -This is not a fix for acpi_is_indirect_io_slave() as I thought at first,
>>   acpi_is_indirect_io_slave() calls acpi_match_device_ids() on its parent
>>   device, where status is already set properly. Rewrite the commit message
>>   accordingly.
> 
> I've applied the v4 of this patch and I don't think there are any
> changes from it here.

Correct, there were only changes to the 4th patch in the series.

> As for the rest of the series I'll wait from comments from Wolfram and
> the other reviewers.

Ok, note if you've taken patch 1 you may also want to take patch 3 which
is an ACPI code cleanup made possible by patch 1 and otherwise is
unrelated.

Regards,

Hans

Re: [PATCH v5 1/4] ACPI / scan: Initialize status to ACPI_STA_DEFAULT

[Andy Shevchenko]

On Thu, Aug 9, 2018 at 12:39 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> On 09-08-18 11:35, Rafael J. Wysocki wrote:
>> On Thu, Aug 9, 2018 at 11:15 AM, Hans de Goede <hdegoede@redhat.com>
>> wrote:

>> I've applied the v4 of this patch and I don't think there are any
>> changes from it here.
>
>
> Correct, there were only changes to the 4th patch in the series.
>
>> As for the rest of the series I'll wait from comments from Wolfram and
>> the other reviewers.
>
>
> Ok, note if you've taken patch 1 you may also want to take patch 3 which
> is an ACPI code cleanup made possible by patch 1 and otherwise is
> unrelated.

I'm under impression Rafael is going to take entire series (at least
for patch 4 I'm expecting to give an Ack).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 1/4] ACPI / scan: Initialize status to ACPI_STA_DEFAULT

[Hans de Goede]

Hi,

On 09-08-18 11:51, Andy Shevchenko wrote:
> On Thu, Aug 9, 2018 at 12:39 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> On 09-08-18 11:35, Rafael J. Wysocki wrote:
>>> On Thu, Aug 9, 2018 at 11:15 AM, Hans de Goede <hdegoede@redhat.com>
>>> wrote:
> 
>>> I've applied the v4 of this patch and I don't think there are any
>>> changes from it here.
>>
>>
>> Correct, there were only changes to the 4th patch in the series.
>>
>>> As for the rest of the series I'll wait from comments from Wolfram and
>>> the other reviewers.
>>
>>
>> Ok, note if you've taken patch 1 you may also want to take patch 3 which
>> is an ACPI code cleanup made possible by patch 1 and otherwise is
>> unrelated.
> 
> I'm under impression Rafael is going to take entire series (at least
> for patch 4 I'm expecting to give an Ack).

As I mentioned in the coverletter, my idea was to have Rafael take
patches 1-3 and then merge the 4th patch through the platform/x86
tree. There are only runtime dependencies between the 2 parts and
merging them independently should not cause any issues.

Regards,

Hans

Re: [PATCH v5 1/4] ACPI / scan: Initialize status to ACPI_STA_DEFAULT

[Rafael J. Wysocki]

On Thu, Aug 9, 2018 at 11:58 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 09-08-18 11:51, Andy Shevchenko wrote:
>>
>> On Thu, Aug 9, 2018 at 12:39 PM, Hans de Goede <hdegoede@redhat.com>
>> wrote:
>>>
>>> On 09-08-18 11:35, Rafael J. Wysocki wrote:
>>>>
>>>> On Thu, Aug 9, 2018 at 11:15 AM, Hans de Goede <hdegoede@redhat.com>
>>>> wrote:
>>
>>
>>>> I've applied the v4 of this patch and I don't think there are any
>>>> changes from it here.
>>>
>>>
>>>
>>> Correct, there were only changes to the 4th patch in the series.
>>>
>>>> As for the rest of the series I'll wait from comments from Wolfram and
>>>> the other reviewers.
>>>
>>>
>>>
>>> Ok, note if you've taken patch 1 you may also want to take patch 3 which
>>> is an ACPI code cleanup made possible by patch 1 and otherwise is
>>> unrelated.
>>
>>
>> I'm under impression Rafael is going to take entire series (at least
>> for patch 4 I'm expecting to give an Ack).
>
>
> As I mentioned in the coverletter, my idea was to have Rafael take
> patches 1-3 and then merge the 4th patch through the platform/x86
> tree. There are only runtime dependencies between the 2 parts and
> merging them independently should not cause any issues.

I can apply the 4th one too if it is ACKed by everyone with a vested interest.

Re: [PATCH v5 4/4] platform/x86: Add ACPI i2c-multi-instantiate pseudo driver

[Wolfram Sang]

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

On Thu, Aug 09, 2018 at 11:15:58AM +0200, Hans de Goede wrote:
> On systems with ACPI instantiated i2c-clients, normally there is 1 fw_node
> per i2c-device and that fw-node contains 1 I2cSerialBus resource for that 1
> i2c-device.
> 
> But in some rare cases the manufacturer has decided to describe multiple
> i2c-devices in a single ACPI fwnode with multiple I2cSerialBus resources.
> 
> An earlier attempt to fix this in the i2c-core resulted in a lot of extra
> code to support this corner-case.
> 
> This commit introduces a new i2c-multi-instantiate driver which fixes this
> in a different way. This new driver can be built as a module which will
> only loaded on affected systems.
> 
> This driver will instantiate a new i2c-client per I2cSerialBus resource,
> using the driver_data from the acpi_device_id it is binding to to tell it
> which chip-type (and optional irq-resource) to use when instantiating.
> 
> Note this driver depends on a platform device being instantiated for the
> ACPI fwnode, see the i2c_multi_instantiate_ids list of ACPI device-ids in
> drivers/acpi/scan.c: acpi_device_enumeration_by_parent().
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

From my I2C PoV:

Acked-by: Wolfram Sang <wsa@the-dreams.de>


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

Re: [PATCH v5 1/4] ACPI / scan: Initialize status to ACPI_STA_DEFAULT

[Wolfram Sang]

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


> As for the rest of the series I'll wait from comments from Wolfram and
> the other reviewers.

Well, I acked patch 4 now because I like the general approach. I can't
say much about the rest since this is mostly ACPI/FW related.

Thanks Hans, for keeping at it.


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

Re: [PATCH v5 2/4] ACPI / scan: Create platform device for fwnodes with multiple i2c devices

[Andy Shevchenko]

On Thu, 2018-08-09 at 11:15 +0200, Hans de Goede wrote:
> Some devices have multiple I2cSerialBus resources and for things to
> work
> an i2c-client must be instantiated for each, each with its own
> i2c_device_id.
> 
> Normally we only instantiate an i2c-client for the first resource,
> using
> the ACPI HID as id.
> 
> This commit adds a list of HIDs of devices, which need multiple i2c-
> clients
> instantiated from a single fwnode, to
> acpi_device_enumeration_by_parent and
> makes acpi_device_enumeration_by_parent return false for these devices
> so
> that a platform device will be instantiated.
> 
> This allows the drivers/platform/x86/i2c-multi-instantiate.c driver,
> which
> knows which i2c_device_id to use for each resource, to bind to the
> fwnode
> and initiate an i2c-client for each resource.
> 

I like this approach

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>


> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v3:
> -New patch in v3 of this patch-set
> 
> Changes in v4:
> -Some small code-style fixes
> ---
>  drivers/acpi/scan.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 6799d00dd790..b51913372e21 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1540,6 +1540,18 @@ static bool
> acpi_device_enumeration_by_parent(struct acpi_device *device)
>  {
>  	struct list_head resource_list;
>  	bool is_serial_bus_slave = false;
> +	/*
> +	 * These devices have multiple I2cSerialBus resources and an
> i2c-client
> +	 * must be instantiated for each, each with its own
> i2c_device_id.
> +	 * Normally we only instantiate an i2c-client for the first
> resource,
> +	 * using the ACPI HID as id. These special cases are handled by
> the
> +	 * drivers/platform/x86/i2c-multi-instantiate.c driver, which
> knows
> +	 * which i2c_device_id to use for each resource.
> +	 */
> +	static const struct acpi_device_id i2c_multi_instantiate_ids[] =
> {
> +		{"BSG1160", },
> +		{}
> +	};
>  
>  	if (acpi_is_indirect_io_slave(device))
>  		return true;
> @@ -1551,6 +1563,10 @@ static bool
> acpi_device_enumeration_by_parent(struct acpi_device *device)
>  	     fwnode_property_present(&device->fwnode, "baud")))
>  		return true;
>  
> +	/* Instantiate a pdev for the i2c-multi-instantiate drv to bind
> to */
> +	if (!acpi_match_device_ids(device, i2c_multi_instantiate_ids))
> +		return false;
> +
>  	INIT_LIST_HEAD(&resource_list);
>  	acpi_dev_get_resources(device, &resource_list,
>  			       acpi_check_serial_bus_slave,

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v5 3/4] ACPI / x86-utils: Remove status workaround from acpi_device_always_present()

[Andy Shevchenko]

On Thu, 2018-08-09 at 11:15 +0200, Hans de Goede wrote:
> Now that we init the status field to ACPI_STA_DEFAULT rather then to
> 0,
> the workaround for acpi_match_device_ids() always returning -ENOENT
> when
> status is 0 is no longer needed.
> 

Always good to have only "minus" statistics

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v3:
> -New patch in v3 of this patch-set
> ---
>  drivers/acpi/x86/utils.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/acpi/x86/utils.c b/drivers/acpi/x86/utils.c
> index ec5b0f190231..a4fb97d64b3b 100644
> --- a/drivers/acpi/x86/utils.c
> +++ b/drivers/acpi/x86/utils.c
> @@ -103,13 +103,9 @@ static const struct always_present_id
> always_present_ids[] = {
>  
>  bool acpi_device_always_present(struct acpi_device *adev)
>  {
> -	u32 *status = (u32 *)&adev->status;
> -	u32 old_status = *status;
>  	bool ret = false;
>  	unsigned int i;
>  
> -	/* acpi_match_device_ids checks status, so set it to default */
> -	*status = ACPI_STA_DEFAULT;
>  	for (i = 0; i < ARRAY_SIZE(always_present_ids); i++) {
>  		if (acpi_match_device_ids(adev,
> always_present_ids[i].hid))
>  			continue;
> @@ -125,15 +121,9 @@ bool acpi_device_always_present(struct
> acpi_device *adev)
>  		    !dmi_check_system(always_present_ids[i].dmi_ids))
>  			continue;
>  
> -		if (old_status != ACPI_STA_DEFAULT) /* Log only once */
> -			dev_info(&adev->dev,
> -				 "Device [%s] is in always present
> list\n",
> -				 adev->pnp.bus_id);
> -
>  		ret = true;
>  		break;
>  	}
> -	*status = old_status;
>  
>  	return ret;
>  }

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v5 4/4] platform/x86: Add ACPI i2c-multi-instantiate pseudo driver

[Andy Shevchenko]

On Thu, 2018-08-09 at 11:15 +0200, Hans de Goede wrote:
> On systems with ACPI instantiated i2c-clients, normally there is 1
> fw_node
> per i2c-device and that fw-node contains 1 I2cSerialBus resource for
> that 1
> i2c-device.
> 
> But in some rare cases the manufacturer has decided to describe
> multiple
> i2c-devices in a single ACPI fwnode with multiple I2cSerialBus
> resources.
> 
> An earlier attempt to fix this in the i2c-core resulted in a lot of
> extra
> code to support this corner-case.
> 
> This commit introduces a new i2c-multi-instantiate driver which fixes
> this
> in a different way. This new driver can be built as a module which
> will
> only loaded on affected systems.
> 
> This driver will instantiate a new i2c-client per I2cSerialBus
> resource,
> using the driver_data from the acpi_device_id it is binding to to tell
> it
> which chip-type (and optional irq-resource) to use when instantiating.
> 
> Note this driver depends on a platform device being instantiated for
> the
> ACPI fwnode, see the i2c_multi_instantiate_ids list of ACPI device-ids 
> in
> drivers/acpi/scan.c: acpi_device_enumeration_by_parent().
> 

Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

(From PDx86 prospective)

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Rebase on top of 4.18-rc2
> 
> Changes in v3:
> -Change from an i2c-driver using a hack to allow having multiple i2c
> clients
>  at the same address to a platform-driver
> 
> Changes in v4:
> -Tweak MAINTAINERS entry a bit
> 
> Changes in v5:
> -s/no_clients/num_clients/
> -Change patch Subject prefix to platform/x86
> ---
>  MAINTAINERS                                  |   6 +
>  drivers/platform/x86/Kconfig                 |  11 ++
>  drivers/platform/x86/Makefile                |   1 +
>  drivers/platform/x86/i2c-multi-instantiate.c | 131
> +++++++++++++++++++
>  4 files changed, 149 insertions(+)
>  create mode 100644 drivers/platform/x86/i2c-multi-instantiate.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9b377508f24f..dbe7836e4f6b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -367,6 +367,12 @@ L:	linux-acpi@vger.kernel.org
>  S:	Maintained
>  F:	drivers/acpi/arm64
>  
> +ACPI I2C MULTI INSTANTIATE DRIVER
> +M:	Hans de Goede <hdegoede@redhat.com>
> +L:	platform-driver-x86@vger.kernel.org
> +S:	Maintained
> +F:	drivers/platform/x86/i2c-multi-instantiate.c
> +
>  ACPI PMIC DRIVERS
>  M:	"Rafael J. Wysocki" <rjw@rjwysocki.net>
>  M:	Len Brown <lenb@kernel.org>
> diff --git a/drivers/platform/x86/Kconfig
> b/drivers/platform/x86/Kconfig
> index 85a93453237c..64c82592d4b6 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -1219,6 +1219,17 @@ config INTEL_CHTDC_TI_PWRBTN
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called intel_chtdc_ti_pwrbtn.
>  
> +config I2C_MULTI_INSTANTIATE
> +	tristate "I2C multi instantiate pseudo device driver"
> +	depends on I2C && ACPI
> +	help
> +	  Some ACPI-based systems list multiple i2c-devices in a single
> ACPI
> +	  firmware-node. This driver will instantiate separate i2c-
> clients
> +	  for each device in the firmware-node.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called i2c-multi-instantiate.
> +
>  endif # X86_PLATFORM_DEVICES
>  
>  config PMC_ATOM
> diff --git a/drivers/platform/x86/Makefile
> b/drivers/platform/x86/Makefile
> index 8d9477114fb5..e6d1becf81ce 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -91,3 +91,4 @@ obj-$(CONFIG_PMC_ATOM)		+= pmc_atom.o
>  obj-$(CONFIG_MLX_PLATFORM)	+= mlx-platform.o
>  obj-$(CONFIG_INTEL_TURBO_MAX_3) += intel_turbo_max_3.o
>  obj-$(CONFIG_INTEL_CHTDC_TI_PWRBTN)	+= intel_chtdc_ti_pwrbtn.o
> +obj-$(CONFIG_I2C_MULTI_INSTANTIATE)	+= i2c-multi-instantiate.o
> diff --git a/drivers/platform/x86/i2c-multi-instantiate.c
> b/drivers/platform/x86/i2c-multi-instantiate.c
> new file mode 100644
> index 000000000000..75c00630ecde
> --- /dev/null
> +++ b/drivers/platform/x86/i2c-multi-instantiate.c
> @@ -0,0 +1,131 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * I2C multi-instantiate driver, pseudo driver to instantiate
> multiple
> + * i2c-clients from a single fwnode.
> + *
> + * Copyright 2018 Hans de Goede <hdegoede@redhat.com>
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +struct i2c_inst_data {
> +	const char *type;
> +	int irq_idx;
> +};
> +
> +struct i2c_multi_inst_data {
> +	int num_clients;
> +	struct i2c_client *clients[0];
> +};
> +
> +static int i2c_multi_inst_probe(struct platform_device *pdev)
> +{
> +	struct i2c_multi_inst_data *multi;
> +	const struct acpi_device_id *match;
> +	const struct i2c_inst_data *inst_data;
> +	struct i2c_board_info board_info = {};
> +	struct device *dev = &pdev->dev;
> +	struct acpi_device *adev;
> +	char name[32];
> +	int i, ret;
> +
> +	match = acpi_match_device(dev->driver->acpi_match_table, dev);
> +	if (!match) {
> +		dev_err(dev, "Error ACPI match data is missing\n");
> +		return -ENODEV;
> +	}
> +	inst_data = (const struct i2c_inst_data *)match->driver_data;
> +
> +	adev = ACPI_COMPANION(dev);
> +
> +	/* Count number of clients to instantiate */
> +	for (i = 0; inst_data[i].type; i++) {}
> +
> +	multi = devm_kmalloc(dev,
> +			offsetof(struct i2c_multi_inst_data,
> clients[i]),
> +			GFP_KERNEL);
> +	if (!multi)
> +		return -ENOMEM;
> +
> +	multi->num_clients = i;
> +
> +	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);
> +		board_info.dev_name = name;
> +		board_info.irq = 0;
> +		if (inst_data[i].irq_idx != -1) {
> +			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].irq_idx, ret);
> +				goto error;
> +			}
> +			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);
> +			ret = -ENODEV;
> +			goto error;
> +		}
> +	}
> +
> +	platform_set_drvdata(pdev, multi);
> +	return 0;
> +
> +error:
> +	while (--i >= 0)
> +		i2c_unregister_device(multi->clients[i]);
> +
> +	return ret;
> +}
> +
> +static int i2c_multi_inst_remove(struct platform_device *pdev)
> +{
> +	struct i2c_multi_inst_data *multi = platform_get_drvdata(pdev);
> +	int i;
> +
> +	for (i = 0; i < multi->num_clients; i++)
> +		i2c_unregister_device(multi->clients[i]);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_inst_data bsg1160_data[]  = {
> +	{ "bmc150_accel", 0 },
> +	{ "bmc150_magn", -1 },
> +	{ "bmg160", -1 },
> +	{}
> +};
> +
> +/*
> + * 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 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, i2c_multi_inst_acpi_ids);
> +
> +static struct platform_driver i2c_multi_inst_driver = {
> +	.driver	= {
> +		.name = "I2C multi instantiate pseudo device driver",
> +		.acpi_match_table = ACPI_PTR(i2c_multi_inst_acpi_ids),
> +	},
> +	.probe = i2c_multi_inst_probe,
> +	.remove = i2c_multi_inst_remove,
> +};
> +module_platform_driver(i2c_multi_inst_driver);
> +
> +MODULE_DESCRIPTION("I2C multi instantiate pseudo device driver");
> +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
> +MODULE_LICENSE("GPL");

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v5 4/4] platform/x86: Add ACPI i2c-multi-instantiate pseudo driver

[Heikki Krogerus]

On Thu, Aug 09, 2018 at 11:15:58AM +0200, Hans de Goede wrote:
> On systems with ACPI instantiated i2c-clients, normally there is 1 fw_node
> per i2c-device and that fw-node contains 1 I2cSerialBus resource for that 1
> i2c-device.
> 
> But in some rare cases the manufacturer has decided to describe multiple
> i2c-devices in a single ACPI fwnode with multiple I2cSerialBus resources.
> 
> An earlier attempt to fix this in the i2c-core resulted in a lot of extra
> code to support this corner-case.
> 
> This commit introduces a new i2c-multi-instantiate driver which fixes this
> in a different way. This new driver can be built as a module which will
> only loaded on affected systems.
> 
> This driver will instantiate a new i2c-client per I2cSerialBus resource,
> using the driver_data from the acpi_device_id it is binding to to tell it
> which chip-type (and optional irq-resource) to use when instantiating.
> 
> Note this driver depends on a platform device being instantiated for the
> ACPI fwnode, see the i2c_multi_instantiate_ids list of ACPI device-ids in
> drivers/acpi/scan.c: acpi_device_enumeration_by_parent().
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---

<snip>

> +static int i2c_multi_inst_probe(struct platform_device *pdev)
> +{
> +	struct i2c_multi_inst_data *multi;
> +	const struct acpi_device_id *match;
> +	const struct i2c_inst_data *inst_data;
> +	struct i2c_board_info board_info = {};
> +	struct device *dev = &pdev->dev;
> +	struct acpi_device *adev;
> +	char name[32];
> +	int i, ret;
> +
> +	match = acpi_match_device(dev->driver->acpi_match_table, dev);
> +	if (!match) {
> +		dev_err(dev, "Error ACPI match data is missing\n");
> +		return -ENODEV;
> +	}
> +	inst_data = (const struct i2c_inst_data *)match->driver_data;
> +
> +	adev = ACPI_COMPANION(dev);
> +
> +	/* Count number of clients to instantiate */
> +	for (i = 0; inst_data[i].type; i++) {}
> +
> +	multi = devm_kmalloc(dev,
> +			offsetof(struct i2c_multi_inst_data, clients[i]),
> +			GFP_KERNEL);
> +	if (!multi)
> +		return -ENOMEM;
> +
> +	multi->num_clients = i;
> +
> +	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);
> +		board_info.dev_name = name;
> +		board_info.irq = 0;
> +		if (inst_data[i].irq_idx != -1) {
> +			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].irq_idx, ret);
> +				goto error;

This seems to assume that we always have GpioInt with assigned for
these devices, but that's wrong. This needs to work with normal
Interrupt type resources as well.

The TI USB PD controller instances (HID 3515) for exmaple have normal
Interrupts assigned to them

Why not use the "irq_idx" with normal interrupts, and add a new member
for the GpioInts, something like gpio_irq_idx?

> +			}
> +			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);
> +			ret = -ENODEV;
> +			goto error;
> +		}
> +	}
> +
> +	platform_set_drvdata(pdev, multi);
> +	return 0;
> +
> +error:
> +	while (--i >= 0)
> +		i2c_unregister_device(multi->clients[i]);
> +
> +	return ret;
> +}

Thanks,

-- 
heikki

Re: [PATCH v5 4/4] platform/x86: Add ACPI i2c-multi-instantiate pseudo driver

[Hans de Goede]

Hi,

On 09-08-18 13:09, Heikki Krogerus wrote:
> On Thu, Aug 09, 2018 at 11:15:58AM +0200, Hans de Goede wrote:
>> On systems with ACPI instantiated i2c-clients, normally there is 1 fw_node
>> per i2c-device and that fw-node contains 1 I2cSerialBus resource for that 1
>> i2c-device.
>>
>> But in some rare cases the manufacturer has decided to describe multiple
>> i2c-devices in a single ACPI fwnode with multiple I2cSerialBus resources.
>>
>> An earlier attempt to fix this in the i2c-core resulted in a lot of extra
>> code to support this corner-case.
>>
>> This commit introduces a new i2c-multi-instantiate driver which fixes this
>> in a different way. This new driver can be built as a module which will
>> only loaded on affected systems.
>>
>> This driver will instantiate a new i2c-client per I2cSerialBus resource,
>> using the driver_data from the acpi_device_id it is binding to to tell it
>> which chip-type (and optional irq-resource) to use when instantiating.
>>
>> Note this driver depends on a platform device being instantiated for the
>> ACPI fwnode, see the i2c_multi_instantiate_ids list of ACPI device-ids in
>> drivers/acpi/scan.c: acpi_device_enumeration_by_parent().
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
> 
> <snip>
> 
>> +static int i2c_multi_inst_probe(struct platform_device *pdev)
>> +{
>> +	struct i2c_multi_inst_data *multi;
>> +	const struct acpi_device_id *match;
>> +	const struct i2c_inst_data *inst_data;
>> +	struct i2c_board_info board_info = {};
>> +	struct device *dev = &pdev->dev;
>> +	struct acpi_device *adev;
>> +	char name[32];
>> +	int i, ret;
>> +
>> +	match = acpi_match_device(dev->driver->acpi_match_table, dev);
>> +	if (!match) {
>> +		dev_err(dev, "Error ACPI match data is missing\n");
>> +		return -ENODEV;
>> +	}
>> +	inst_data = (const struct i2c_inst_data *)match->driver_data;
>> +
>> +	adev = ACPI_COMPANION(dev);
>> +
>> +	/* Count number of clients to instantiate */
>> +	for (i = 0; inst_data[i].type; i++) {}
>> +
>> +	multi = devm_kmalloc(dev,
>> +			offsetof(struct i2c_multi_inst_data, clients[i]),
>> +			GFP_KERNEL);
>> +	if (!multi)
>> +		return -ENOMEM;
>> +
>> +	multi->num_clients = i;
>> +
>> +	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);
>> +		board_info.dev_name = name;
>> +		board_info.irq = 0;
>> +		if (inst_data[i].irq_idx != -1) {
>> +			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].irq_idx, ret);
>> +				goto error;
> 
> This seems to assume that we always have GpioInt with assigned for
> these devices, but that's wrong. This needs to work with normal
> Interrupt type resources as well.
> 
> The TI USB PD controller instances (HID 3515) for exmaple have normal
> Interrupts assigned to them
> 
> Why not use the "irq_idx" with normal interrupts, and add a new member
> for the GpioInts, something like gpio_irq_idx?

I don't have any use-cases to test using normal interrupts, so I
will leave adding support for that to the first patch adding support
for a device which needs that, such as the HID3515 you mention.

I do agree that it is good to prepare for adding this later, so
I'm going to post a v6 doing s/irq_idx/gpio_irq_idx/

Regards,

Hans



> 
>> +			}
>> +			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);
>> +			ret = -ENODEV;
>> +			goto error;
>> +		}
>> +	}
>> +
>> +	platform_set_drvdata(pdev, multi);
>> +	return 0;
>> +
>> +error:
>> +	while (--i >= 0)
>> +		i2c_unregister_device(multi->clients[i]);
>> +
>> +	return ret;
>> +}
> 
> Thanks,
> 

Re: [PATCH v5 1/4] ACPI / scan: Initialize status to ACPI_STA_DEFAULT

[Hans de Goede]

Hi,

On 09-08-18 11:59, Rafael J. Wysocki wrote:
> On Thu, Aug 9, 2018 at 11:58 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 09-08-18 11:51, Andy Shevchenko wrote:
>>>
>>> On Thu, Aug 9, 2018 at 12:39 PM, Hans de Goede <hdegoede@redhat.com>
>>> wrote:
>>>>
>>>> On 09-08-18 11:35, Rafael J. Wysocki wrote:
>>>>>
>>>>> On Thu, Aug 9, 2018 at 11:15 AM, Hans de Goede <hdegoede@redhat.com>
>>>>> wrote:
>>>
>>>
>>>>> I've applied the v4 of this patch and I don't think there are any
>>>>> changes from it here.
>>>>
>>>>
>>>>
>>>> Correct, there were only changes to the 4th patch in the series.
>>>>
>>>>> As for the rest of the series I'll wait from comments from Wolfram and
>>>>> the other reviewers.
>>>>
>>>>
>>>>
>>>> Ok, note if you've taken patch 1 you may also want to take patch 3 which
>>>> is an ACPI code cleanup made possible by patch 1 and otherwise is
>>>> unrelated.
>>>
>>>
>>> I'm under impression Rafael is going to take entire series (at least
>>> for patch 4 I'm expecting to give an Ack).
>>
>>
>> As I mentioned in the coverletter, my idea was to have Rafael take
>> patches 1-3 and then merge the 4th patch through the platform/x86
>> tree. There are only runtime dependencies between the 2 parts and
>> merging them independently should not cause any issues.
> 
> I can apply the 4th one too if it is ACKed by everyone with a vested interest.

That works for me, note I'm about to send out a v6 (with only changes to
the 4th patch), so hold of a bit with merging this please.

Andy does your ack for the 4th patch mean you're ok with Rafael merging
this?

Regards,

Hans

Re: [PATCH v5 1/4] ACPI / scan: Initialize status to ACPI_STA_DEFAULT

[Andy Shevchenko]

On Thu, 2018-08-09 at 13:36 +0200, Hans de Goede wrote:


> Andy does your ack for the 4th patch mean you're ok with Rafael
> merging
> this?

Yes.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy