[PATCH v3 0/4] ACPI bugfix + i2c-multi-instantiate pseudo driver

[PATCH v3 0/4] ACPI bugfix + i2c-multi-instantiate pseudo driver

[Hans de Goede]

Hi All,

So here is my third attempt at dealing with multiple i2c devices being
described in a single ACPI fwnode.

Like the second attemp This is still based on having a special driver for
these corner cases which can be a module and only loaded when on hardware
which needs this.

New this time around is that this now is a platform driver, avoiding the
need for the I2C_CLIENT_IGNORE_BUSY hack and having 2 i2c-clients at
the address of the first i2c ACPI resource.

While working on the ACPI changes for enumerating ACPI nodes which
are handled by the new i2c-multi-instantiate driver as platform devices
I found a bug caused by a combination of my:
"ACPI / scan: Use acpi_bus_get_status() to initialize ACPI_TYPE_DEVICE devs"
and the somewhat (but not much) later added:
"ACPI / scan: Do not enumerate Indirect IO host children"
commits, specifically the second one does not work because of the first
one, I guess it was tested against a kernel without the first commit.
My changes for enumerating relevant ACPI nodes as platform devices
hit the same problem.

Thus the first patch of this series is an ACPI bugfix for that problem.
The second patch changes acpi/scan.c to enumerate relevant ACPI nodes as
platform devices.
The third patch is an ACPI cleanup patch made possible by the first patch.
All 3 these patches are new in this version of this series.

And finally the fourth patch is the actual i2c-multi-instantiate driver,
now as platform driver living under drivers/platform/x86 .

Regards,

Hans

[PATCH v3 1/4] ACPI / scan: Fix acpi_is_indirect_io_slave() always returning false

[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 status is 0 when acpi_device_enumeration_by_parent()
gets called which makes the acpi_match_device_ids() call in
acpi_is_indirect_io_slave() always return -ENOENT.

This commit fixes this by making acpi_bus_type_and_status() set status to
ACPI_STA_DEFAULT until acpi_add_single_object() calls acpi_bus_get_status()

Fixes: dfda4492322e ("ACPI / scan: Do not enumerate Indirect IO host children")
Cc: stable@vger.kernel.org
Cc: John Garry <john.garry@huawei.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Dann Frazier <dann.frazier@canonical.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 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 v3 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>
---
 drivers/acpi/scan.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 6799d00dd790..44068066d269 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", 0},
+		{"", 0},
+	};
 
 	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 v3 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>
---
 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 v3 4/4] i2c: Add 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>
---
 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..09e16d76cab7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6657,6 +6657,12 @@ F:	drivers/i2c/i2c-mux.c
 F:	drivers/i2c/muxes/
 F:	include/linux/i2c-mux.h
 
+I2C MULTI INSTANTIATE DRIVER
+M:	Hans de Goede <hdegoede@redhat.com>
+L:	linux-i2c@vger.kernel.org
+S:	Maintained
+F:	drivers/drivers/platform/x86/i2c-multi-instantiate.c
+
 I2C MV64XXX MARVELL AND ALLWINNER DRIVER
 M:	Gregory CLEMENT <gregory.clement@bootlin.com>
 L:	linux-i2c@vger.kernel.org
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 85a93453237c..8256629f58f7 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 bases 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..4db4b8cabfc9
--- /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 no_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->no_clients = i;
+
+	for (i = 0; i < multi->no_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->no_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 v3 1/4] ACPI / scan: Fix acpi_is_indirect_io_slave() always returning false

[John Garry]

On 07/08/2018 09:05, Hans de Goede 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 status is 0 when acpi_device_enumeration_by_parent()
> gets called which makes the acpi_match_device_ids() call in
> acpi_is_indirect_io_slave() always return -ENOENT.

For for the (child) device the status may be zero, but how about the 
parent device (which is what we care about)? As I see, we would have set 
the parent device status present field at this stage.

Thanks,
John

>
> This commit fixes this by making acpi_bus_type_and_status() set status to
> ACPI_STA_DEFAULT until acpi_add_single_object() calls acpi_bus_get_status()
>
> Fixes: dfda4492322e ("ACPI / scan: Do not enumerate Indirect IO host children")
> Cc: stable@vger.kernel.org
> Cc: John Garry <john.garry@huawei.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Dann Frazier <dann.frazier@canonical.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  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;
>

Re: [PATCH v3 1/4] ACPI / scan: Fix acpi_is_indirect_io_slave() always returning false

[Hans de Goede]

Hi,

On 07-08-18 12:05, John Garry wrote:
> On 07/08/2018 09:05, Hans de Goede 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 status is 0 when acpi_device_enumeration_by_parent()
>> gets called which makes the acpi_match_device_ids() call in
>> acpi_is_indirect_io_slave() always return -ENOENT.
> 
> For for the (child) device the status may be zero, but how about the parent device (which is what we care about)? As I see, we would have set the parent device status present field at this stage.

Ah I did not notice you were calling acpi_match_device_ids() on the parents.
Yes I think you are right. This commit is still a necessary part of the
series, but it needs a completely new commit message no longer
referencing acpi_is_indirect_io_slave().

I will submit a v4 of this patch-set with new commit message for this patch.

Regards,

Hans






> 
> Thanks,
> John
> 
>>
>> This commit fixes this by making acpi_bus_type_and_status() set status to
>> ACPI_STA_DEFAULT until acpi_add_single_object() calls acpi_bus_get_status()
>>
>> Fixes: dfda4492322e ("ACPI / scan: Do not enumerate Indirect IO host children")
>> Cc: stable@vger.kernel.org
>> Cc: John Garry <john.garry@huawei.com>
>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>> Cc: Dann Frazier <dann.frazier@canonical.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  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;
>>
> 
> 

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

[Andy Shevchenko]

On Tue, 2018-08-07 at 10:05 +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.
> 

> +	/*
> +	 * 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", 0},
> +		{"", 0},
> +	};

Style nits:
- can we move it outside of function?
- terminator better without comma
- is this existing style in the file and / or files in this folder for
IDs? (I mean unnecessary 0:s and empty string?

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

Re: [PATCH v3 4/4] i2c: Add multi-instantiate pseudo driver

[Wolfram Sang]

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

Hi Hans,

> +I2C MULTI INSTANTIATE DRIVER
> +M:	Hans de Goede <hdegoede@redhat.com>
> +L:	linux-i2c@vger.kernel.org

Since this driver is about FW bugs, I wonder if
platform-driver-x86@vger.kernel.org isn't the better list here?

> +S:	Maintained
> +F:	drivers/drivers/platform/x86/i2c-multi-instantiate.c

I2C parts of the rest looks good from a glimpse...


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

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

[Hans de Goede]

Hi,

On 07-08-18 13:19, Andy Shevchenko wrote:
> On Tue, 2018-08-07 at 10:05 +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.
>>
> 
>> +	/*
>> +	 * 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", 0},
>> +		{"", 0},
>> +	};
> 
> Style nits:
> - can we move it outside of function?

Sure, but there are 2 existing users of an array of acpi_device_id-s
combined with an acpi_match_device_ids() call and both have the array
inside the function, so for consistency it seems better to keep it
where it is.

> - terminator better without comma

Agreed, will fix for v4.

> - is this existing style in the file and / or files in this folder for
> IDs? (I mean unnecessary 0:s and empty string?

It seems that all variants one can come up with are already used inside
this single file.

I agree that less is more, so I will change this to:

         static const struct acpi_device_id i2c_multi_instantiate_ids[] = {
                 {"BSG1160", },
                 {}
         };

For v4.

Regards,

Hans

Re: [PATCH v3 4/4] i2c: Add multi-instantiate pseudo driver

[Hans de Goede]

Hi,

On 07-08-18 13:27, Wolfram Sang wrote:
> Hi Hans,
> 
>> +I2C MULTI INSTANTIATE DRIVER
>> +M:	Hans de Goede <hdegoede@redhat.com>
>> +L:	linux-i2c@vger.kernel.org
> 
> Since this driver is about FW bugs, I wonder if
> platform-driver-x86@vger.kernel.org isn't the better list here?

Ok, fixed for v4.

>> +S:	Maintained
>> +F:	drivers/drivers/platform/x86/i2c-multi-instantiate.c
> 
> I2C parts of the rest looks good from a glimpse...

Thanks.

Regards,

Hans

Re: [PATCH v3 4/4] i2c: Add multi-instantiate pseudo driver

[Andy Shevchenko]

On Tue, 2018-08-07 at 13:33 +0200, Hans de Goede wrote:
> Hi,
> 
> On 07-08-18 13:27, Wolfram Sang wrote:
> > Hi Hans,
> > 
> > > +I2C MULTI INSTANTIATE DRIVER

In the case below, I think it would be slightly better to mention ACPI
here, something like

ACPI I2C ...

Otherwise it sounds a bit confusing why it's in PDx86.

> > > +M:	Hans de Goede <hdegoede@redhat.com>
> > > +L:	linux-i2c@vger.kernel.org
> > 
> > Since this driver is about FW bugs, I wonder if
> > platform-driver-x86@vger.kernel.org isn't the better list here?
> 
> Ok, fixed for v4.
> 
> > > +S:	Maintained
> > > +F:	drivers/drivers/platform/x86/i2c-multi-instantiate.c
> > 
> > I2C parts of the rest looks good from a glimpse...
> 
> Thanks.
> 
> Regards,
> 
> Hans

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

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

[Andy Shevchenko]

On Tue, 2018-08-07 at 13:29 +0200, Hans de Goede wrote:
> Hi,
> 
> On 07-08-18 13:19, Andy Shevchenko wrote:
> > On Tue, 2018-08-07 at 10:05 +0200, Hans de Goede wrote:

> > > +	/*
> > > +	 * 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", 0},
> > > +		{"", 0},
> > > +	};
> > 
> > Style nits:
> > - can we move it outside of function?
> 
> Sure, but there are 2 existing users of an array of acpi_device_id-s
> combined with an acpi_match_device_ids() call and both have the array
> inside the function, so for consistency it seems better to keep it
> where it is.

Hmm... OK.

> > - is this existing style in the file and / or files in this folder
> > for
> > IDs? (I mean unnecessary 0:s and empty string?
> 
> It seems that all variants one can come up with are already used
> inside
> this single file.

Ah, that's sad.

> I agree that less is more, so I will change this to:
> 
>          static const struct acpi_device_id
> i2c_multi_instantiate_ids[] = {

>                  {"BSG1160", },
>                  {}
>          };

In case if it mimics already existing style, looks quite good to me
(otherwise perhaps comma inside {} can also be removed).

> 
> For v4.

Does it make sense to test v3 on your opinion? Or better to wait for v4?

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

Re: [PATCH v3 4/4] i2c: Add multi-instantiate pseudo driver

[John Garry]

On 07/08/2018 09:05, 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>
> ---
>  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..09e16d76cab7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6657,6 +6657,12 @@ F:	drivers/i2c/i2c-mux.c
>  F:	drivers/i2c/muxes/
>  F:	include/linux/i2c-mux.h
>
> +I2C MULTI INSTANTIATE DRIVER
> +M:	Hans de Goede <hdegoede@redhat.com>
> +L:	linux-i2c@vger.kernel.org
> +S:	Maintained
> +F:	drivers/drivers/platform/x86/i2c-multi-instantiate.c

This path looks incorrect

> +
>  I2C MV64XXX MARVELL AND ALLWINNER DRIVER
>  M:	Gregory CLEMENT <gregory.clement@bootlin.com>
>  L:	linux-i2c@vger.kernel.org
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 85a93453237c..8256629f58f7 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 bases systems list multiple i2c-devices in a single ACPI

I guess that this should be "Some ACPI-based systems"

> +	  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.
> +

Thanks,
John

Re: [PATCH v3 4/4] i2c: Add multi-instantiate pseudo driver

[Hans de Goede]

Hi,

On 07-08-18 13:45, Andy Shevchenko wrote:
> On Tue, 2018-08-07 at 13:33 +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 07-08-18 13:27, Wolfram Sang wrote:
>>> Hi Hans,
>>>
>>>> +I2C MULTI INSTANTIATE DRIVER
> 
> In the case below, I think it would be slightly better to mention ACPI
> here, something like
> 
> ACPI I2C ...
> 
> Otherwise it sounds a bit confusing why it's in PDx86.

Ok, fixed for the upcoming v4 of this patchset.

Regards,

Hans


> 
>>>> +M:	Hans de Goede <hdegoede@redhat.com>
>>>> +L:	linux-i2c@vger.kernel.org
>>>
>>> Since this driver is about FW bugs, I wonder if
>>> platform-driver-x86@vger.kernel.org isn't the better list here?
>>
>> Ok, fixed for v4.
>>
>>>> +S:	Maintained
>>>> +F:	drivers/drivers/platform/x86/i2c-multi-instantiate.c
>>>
>>> I2C parts of the rest looks good from a glimpse...
>>
>> Thanks.
>>
>> Regards,
>>
>> Hans
> 

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

[Hans de Goede]

Hi,

On 07-08-18 13:49, Andy Shevchenko wrote:
> On Tue, 2018-08-07 at 13:29 +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 07-08-18 13:19, Andy Shevchenko wrote:
>>> On Tue, 2018-08-07 at 10:05 +0200, Hans de Goede wrote:
> 
>>>> +	/*
>>>> +	 * 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", 0},
>>>> +		{"", 0},
>>>> +	};
>>>
>>> Style nits:
>>> - can we move it outside of function?
>>
>> Sure, but there are 2 existing users of an array of acpi_device_id-s
>> combined with an acpi_match_device_ids() call and both have the array
>> inside the function, so for consistency it seems better to keep it
>> where it is.
> 
> Hmm... OK.
> 
>>> - is this existing style in the file and / or files in this folder
>>> for
>>> IDs? (I mean unnecessary 0:s and empty string?
>>
>> It seems that all variants one can come up with are already used
>> inside
>> this single file.
> 
> Ah, that's sad.
> 
>> I agree that less is more, so I will change this to:
>>
>>           static const struct acpi_device_id
>> i2c_multi_instantiate_ids[] = {
> 
>>                   {"BSG1160", },
>>                   {}
>>           };
> 
> In case if it mimics already existing style, looks quite good to me
> (otherwise perhaps comma inside {} can also be removed).
> 
>>
>> For v4.
> 
> Does it make sense to test v3 on your opinion? Or better to wait for v4?

Sorry for being a bit slow to answer, I'm about to send out v4, so probably
best to wait for that now. Note the 2 will be functionally identical,
I mainly fixed / clarified commit messages and the MAINTAINERS entry +
the small style fixed discussed above.

Regards,

Hans

Re: [PATCH v3 4/4] i2c: Add multi-instantiate pseudo driver

[Hans de Goede]

Hi,

On 07-08-18 14:46, John Garry wrote:
> On 07/08/2018 09:05, 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>
>> ---
>>  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..09e16d76cab7 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -6657,6 +6657,12 @@ F:    drivers/i2c/i2c-mux.c
>>  F:    drivers/i2c/muxes/
>>  F:    include/linux/i2c-mux.h
>>
>> +I2C MULTI INSTANTIATE DRIVER
>> +M:    Hans de Goede <hdegoede@redhat.com>
>> +L:    linux-i2c@vger.kernel.org
>> +S:    Maintained
>> +F:    drivers/drivers/platform/x86/i2c-multi-instantiate.c
> 
> This path looks incorrect
> 
>> +
>>  I2C MV64XXX MARVELL AND ALLWINNER DRIVER
>>  M:    Gregory CLEMENT <gregory.clement@bootlin.com>
>>  L:    linux-i2c@vger.kernel.org
>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> index 85a93453237c..8256629f58f7 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 bases systems list multiple i2c-devices in a single ACPI
> 
> I guess that this should be "Some ACPI-based systems"

Thanks, both fixed for v4 of the patchset.

Regards,

Hans


> 
>> +      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.
>> +
> 
> Thanks,
> John
>