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

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

[Hans de Goede]

Hi All,

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

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 v4 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 v4 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 v4 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 v4 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>
---
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
---
 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..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 v4 4/4] i2c: Add multi-instantiate pseudo driver

[Andy Shevchenko]

On Wed, Aug 8, 2018 at 11:30 AM, Hans de Goede <hdegoede@redhat.com> 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().

Thanks for an update! My comments below.

> +struct i2c_inst_data {
> +       const char *type;
> +       int irq_idx;
> +};

> +struct i2c_multi_inst_data {

> +       int no_clients;

Name a bit confusing. What about 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;

Here I see the following:
 - it's kinda unusual use of offsetof(), perhaps i*sizeof() + sizeof()
would be more understandable
 - there is no guard against i == 0

To solve both, it might be like

struct i2c_multi_inst_data {
       int num_clients;
       struct i2c_client *clients;
};

...
multi = devm_kmalloc(sizeof(*multi), GFP_KERNEL);
if (!multi)
 return -ENOMEM;

multi->clients = devm_kcalloc(i, sizeof(*multi->clients), GFP_KERNEL);
if (ZERO_PTR_OR_NULL(multi->clients))
 return -ENOMEM;

But I would like to hear your (other's) opinion(s).

> +
> +       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) {

>= 0 sounds more robust

> +                       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);

irq -> IRQ in the message.

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

It can be simple

while (i--)

> +               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),

We don't need ACPI_PTR for the driver which depends on ACPI.
In the general case we have an inconsistency with variable definition
(might be unused).

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



-- 
With Best Regards,
Andy Shevchenko

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

[Peter Rosin]

On 2018-08-08 11:08, Andy Shevchenko wrote:
> On Wed, Aug 8, 2018 at 11:30 AM, Hans de Goede <hdegoede@redhat.com> 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().
> 
> Thanks for an update! My comments below.
> 
>> +struct i2c_inst_data {
>> +       const char *type;
>> +       int irq_idx;
>> +};
> 
>> +struct i2c_multi_inst_data {
> 
>> +       int no_clients;
> 
> Name a bit confusing. What about 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;
> 
> Here I see the following:
>  - it's kinda unusual use of offsetof(), perhaps i*sizeof() + sizeof()
> would be more understandable
>  - there is no guard against i == 0

I don't see why a guard is needed? *Your* code below needs it, but that
issue is not a concern for the original code. It might however be a
good idea to fail the probe if there are no clients to instantiate, but
that's a different issue...

> 
> To solve both, it might be like
> 
> struct i2c_multi_inst_data {
>        int num_clients;
>        struct i2c_client *clients;
> };
> 
> ...
> multi = devm_kmalloc(sizeof(*multi), GFP_KERNEL);
> if (!multi)
>  return -ENOMEM;
> 
> multi->clients = devm_kcalloc(i, sizeof(*multi->clients), GFP_KERNEL);
> if (ZERO_PTR_OR_NULL(multi->clients))
>  return -ENOMEM;
> 
> But I would like to hear your (other's) opinion(s).

I think using two allocations is a waste in this case.

> 
>> +
>> +       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) {
> 
>> = 0 sounds more robust

But not as flexible/future-proof. Why should 0 be the only valid IRQ index?

Cheers,
Peter

>> +                       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);
> 
> irq -> IRQ in the message.
> 
>> +                               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)
> 
> It can be simple
> 
> while (i--)
> 
>> +               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),
> 
> We don't need ACPI_PTR for the driver which depends on ACPI.
> In the general case we have an inconsistency with variable definition
> (might be unused).
> 
>> +       },
>> +       .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 v4 4/4] i2c: Add multi-instantiate pseudo driver

[Andy Shevchenko]

On Wed, Aug 8, 2018 at 12:47 PM, Peter Rosin <peda@axentia.se> wrote:
> On 2018-08-08 11:08, Andy Shevchenko wrote:
>> On Wed, Aug 8, 2018 at 11:30 AM, Hans de Goede <hdegoede@redhat.com> wrote:

>>> +       /* 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;
>>
>> Here I see the following:
>>  - it's kinda unusual use of offsetof(), perhaps i*sizeof() + sizeof()
>> would be more understandable
>>  - there is no guard against i == 0
>
> I don't see why a guard is needed?

Because there is no point to have a module loaded when there is none
client to serve.

> *Your* code below needs it, but that
> issue is not a concern for the original code.

I can admit that's not a big deal, just making logic slightly more robust.

> It might however be a
> good idea to fail the probe if there are no clients to instantiate, but
> that's a different issue...

That's what I have in mind.

>> multi = devm_kmalloc(sizeof(*multi), GFP_KERNEL);
>> if (!multi)
>>  return -ENOMEM;
>>
>> multi->clients = devm_kcalloc(i, sizeof(*multi->clients), GFP_KERNEL);
>> if (ZERO_PTR_OR_NULL(multi->clients))
>>  return -ENOMEM;
>>
>> But I would like to hear your (other's) opinion(s).
>
> I think using two allocations is a waste in this case.

On the other hand it makes code more readable. With offsetof() it is a
bit hard to get it on the first glance.

>>> +               if (inst_data[i].irq_idx != -1) {
>>
>>> = 0 sounds more robust
>
> But not as flexible/future-proof. Why should 0 be the only valid IRQ index?

Ah, because > is used usually is a quoting character in email you
missed the point.
It was written as >= 0.

-- 
With Best Regards,
Andy Shevchenko

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

[Peter Rosin]

On 2018-08-08 12:01, Andy Shevchenko wrote:
> On Wed, Aug 8, 2018 at 12:47 PM, Peter Rosin <peda@axentia.se> wrote:
>> On 2018-08-08 11:08, Andy Shevchenko wrote:
>>> On Wed, Aug 8, 2018 at 11:30 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> 
>>>> +       /* 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;
>>>
>>> Here I see the following:
>>>  - it's kinda unusual use of offsetof(), perhaps i*sizeof() + sizeof()
>>> would be more understandable
>>>  - there is no guard against i == 0
>>
>> I don't see why a guard is needed?
> 
> Because there is no point to have a module loaded when there is none
> client to serve.
> 
>> *Your* code below needs it, but that
>> issue is not a concern for the original code.
> 
> I can admit that's not a big deal, just making logic slightly more robust.
> 
>> It might however be a
>> good idea to fail the probe if there are no clients to instantiate, but
>> that's a different issue...
> 
> That's what I have in mind.

Ah, but there is no reason what-so-ever for i being zero. The whole point
of the driver is for cases where i > 1. Or to put it bluntly, anyone
defining a struct i2c_inst_data with zero entries deserves to be punished...

>>> multi = devm_kmalloc(sizeof(*multi), GFP_KERNEL);
>>> if (!multi)
>>>  return -ENOMEM;
>>>
>>> multi->clients = devm_kcalloc(i, sizeof(*multi->clients), GFP_KERNEL);
>>> if (ZERO_PTR_OR_NULL(multi->clients))
>>>  return -ENOMEM;
>>>
>>> But I would like to hear your (other's) opinion(s).
>>
>> I think using two allocations is a waste in this case.
> 
> On the other hand it makes code more readable. With offsetof() it is a
> bit hard to get it on the first glance.

The driver is tiny, I think it's good if there is at least one thing that
can be a little bit interesting :-)

>>>> +               if (inst_data[i].irq_idx != -1) {
>>>
>>>> = 0 sounds more robust
>>
>> But not as flexible/future-proof. Why should 0 be the only valid IRQ index?
> 
> Ah, because > is used usually is a quoting character in email you
> missed the point.
> It was written as >= 0.

Ahh, good catch, that explains it.

Cheers,
Peter

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

[Hans de Goede]

Hi,

On 08/08/2018 11:08 AM, Andy Shevchenko wrote:
> On Wed, Aug 8, 2018 at 11:30 AM, Hans de Goede <hdegoede@redhat.com> 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().
> 
> Thanks for an update! My comments below.
> 
>> +struct i2c_inst_data {
>> +       const char *type;
>> +       int irq_idx;
>> +};
> 
>> +struct i2c_multi_inst_data {
> 
>> +       int no_clients;
> 
> Name a bit confusing. What about num_clients?

no is often used as abbreviation for "number of" so this is a quite
normal naming scheme. But if you really want me to I can do a v5
renaming this to 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;
> 
> Here I see the following:
>   - it's kinda unusual use of offsetof()

There are actually plenty of places doing this, this is the normal way
to get the size of a struct which ends with a variable sized array.

I would prefer to keep this as is.

, perhaps i*sizeof() + sizeof()
> would be more understandable
>   - there is no guard against i == 0

i depends on the driver_data, defining a driver_data
where i == 0 is silly, still if this happens nothing bad
will happen, so I see no need for a check for this.

> 
> To solve both, it might be like
> 
> struct i2c_multi_inst_data {
>         int num_clients;
>         struct i2c_client *clients;
> };
> 
> ...
> multi = devm_kmalloc(sizeof(*multi), GFP_KERNEL);
> if (!multi)
>   return -ENOMEM;
> 
> multi->clients = devm_kcalloc(i, sizeof(*multi->clients), GFP_KERNEL);
> if (ZERO_PTR_OR_NULL(multi->clients))
>   return -ENOMEM;
> 
> But I would like to hear your (other's) opinion(s).

Nack for this option, this just makes the code more
complicated and uses allocs instead of 1 for no good
reason IMHO.

Regards,

Hans


> 
>> +
>> +       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) {
> 
>> = 0 sounds more robust
> 
>> +                       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);
> 
> irq -> IRQ in the message.
> 
>> +                               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)
> 
> It can be simple
> 
> while (i--)
> 
>> +               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),
> 
> We don't need ACPI_PTR for the driver which depends on ACPI.
> In the general case we have an inconsistency with variable definition
> (might be unused).
> 
>> +       },
>> +       .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 v4 4/4] i2c: Add multi-instantiate pseudo driver

[Wolfram Sang]

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


$subject still says "i2c:"

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

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

[Hans de Goede]

Hi,

On 08-08-18 19:00, Wolfram Sang wrote:
> 
> $subject still says "i2c:"

Ah, good point I will send a v5 with this fixed, I will also
do s/no_clients/num_clients/ for v5 as Andy requested.

Regards,

Hans