[v2,2/2] i2c: Add multi-instantiate pseudo driver

Message ID 20180701122343.27194-3-hdegoede@redhat.com
State New
Headers show
Series
  • i2c: ACPI: Add multi-instantiate pseudo driver
Related show

Commit Message

Hans de Goede July 1, 2018, 12:23 p.m.
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 commitintroduces 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 means that we will be instantiating 2 i2c_client-s for the first
I2cSerialBus resource, this is not pretty, but since the multi-instantiate
driver does exactly 0 i2c-transfers this is not a problem.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/Kconfig                 |   8 ++
 drivers/i2c/Makefile                |   1 +
 drivers/i2c/i2c-multi-instantiate.c | 125 ++++++++++++++++++++++++++++
 3 files changed, 134 insertions(+)
 create mode 100644 drivers/i2c/i2c-multi-instantiate.c

Comments

Andy Shevchenko July 2, 2018, 1:25 p.m. | #1
On Sun, 2018-07-01 at 14:23 +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 commitintroduces 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 means that we will be instantiating 2 i2c_client-s for the
> first
> I2cSerialBus resource, this is not pretty, but since the multi-
> instantiate
> driver does exactly 0 i2c-transfers this is not a problem.

Hmm... I was thinking about somelike MFD for I2C devices where we have a
real tree, thus, we instantiate parent device (that has nothing to do
with I2C per se) and per each I2cSerialBus resource we would instantiate
its children.

I think it would be slightly cleaner approach (no need to care about
parent device being I2C client).

I dunno if it's possible to implement in a tiny way, though.
Hans de Goede July 2, 2018, 3:10 p.m. | #2
Hi,

On 02-07-18 15:25, Andy Shevchenko wrote:
> On Sun, 2018-07-01 at 14:23 +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 commitintroduces 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 means that we will be instantiating 2 i2c_client-s for the
>> first
>> I2cSerialBus resource, this is not pretty, but since the multi-
>> instantiate
>> driver does exactly 0 i2c-transfers this is not a problem.
> 
> Hmm... I was thinking about somelike MFD for I2C devices where we have a
> real tree, thus, we instantiate parent device (that has nothing to do
> with I2C per se) and per each I2cSerialBus resource we would instantiate
> its children.
> 
> I think it would be slightly cleaner approach (no need to care about
> parent device being I2C client).
> 
> I dunno if it's possible to implement in a tiny way, though.

Not really, one thing which comes to mind is to modify the ACPI
code to enumerate the ACPI fwnode into a platform device as it
normally does. But this means duplicating the list of ACPI HIDs
which is now in the pseudo driver inside the ACPI core and
add special treatment for them there, which I think is not a good
idea.

This would allow us to get rid of the I2C_CLIENT_IGNORE_BUSY,
note that the instantiated i2c devices will still have their
i2c bus controller as parent, not the new ACPI instantiated
platform device, as that is just how the i2c subsys works.

So the only thing this would fix from a user looking into
sysfs to see how devices are connected pov is that we no
longer have the original (pseudo) i2c client instantiated
by the ACPI core to which the "multi-instantiate pseudo driver"
binds.

So all in all I'm afraid that this patch-set is the least
ugly solution I can come up with.

Regards,

Hans

Patch

diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
index efc3354d60ae..12ca950e7549 100644
--- a/drivers/i2c/Kconfig
+++ b/drivers/i2c/Kconfig
@@ -120,6 +120,14 @@  config I2C_SLAVE_EEPROM
 
 endif
 
+config I2C_MULTI_INSTANTIATE
+	tristate "I2C multi instantiate pseudo device driver"
+	depends on 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.
+
 config I2C_DEBUG_CORE
 	bool "I2C Core debugging messages"
 	help
diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
index 72c94c60fdd1..d9c5bae1d64f 100644
--- a/drivers/i2c/Makefile
+++ b/drivers/i2c/Makefile
@@ -16,6 +16,7 @@  obj-$(CONFIG_I2C_MUX)		+= i2c-mux.o
 obj-y				+= algos/ busses/ muxes/
 obj-$(CONFIG_I2C_STUB)		+= i2c-stub.o
 obj-$(CONFIG_I2C_SLAVE_EEPROM)	+= i2c-slave-eeprom.o
+obj-$(CONFIG_I2C_MULTI_INSTANTIATE) += i2c-multi-instantiate.o
 
 ccflags-$(CONFIG_I2C_DEBUG_CORE) := -DDEBUG
 CFLAGS_i2c-core-base.o := -Wno-deprecated-declarations
diff --git a/drivers/i2c/i2c-multi-instantiate.c b/drivers/i2c/i2c-multi-instantiate.c
new file mode 100644
index 000000000000..480b7a694eae
--- /dev/null
+++ b/drivers/i2c/i2c-multi-instantiate.c
@@ -0,0 +1,125 @@ 
+// 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>
+
+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 i2c_client *client)
+{
+	struct i2c_multi_inst_data *multi;
+	const struct i2c_inst_data *inst_data;
+	struct i2c_board_info board_info = {};
+	struct device *dev = &client->dev;
+	struct acpi_device *adev;
+	char name[32];
+	int i, ret;
+
+	inst_data = acpi_device_get_match_data(dev);
+	if (!inst_data) {
+		dev_err(dev, "Error ACPI match data is missing\n");
+		return -ENODEV;
+	}
+
+	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);
+		board_info.flags = (i == 0) ? I2C_CLIENT_IGNORE_BUSY : 0;
+		snprintf(name, sizeof(name), "%s-%s", client->name,
+			 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;
+		}
+	}
+
+	return 0;
+
+error:
+	while (--i >= 0)
+		i2c_unregister_device(multi->clients[i]);
+
+	return ret;
+}
+
+static int i2c_multi_inst_remove(struct i2c_client *i2c)
+{
+	struct i2c_multi_inst_data *multi = i2c_get_clientdata(i2c);
+	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 },
+	{}
+};
+
+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 i2c_driver i2c_multi_inst_driver = {
+	.driver	= {
+		.name = "I2C multi instantiate pseudo device driver",
+		.acpi_match_table = ACPI_PTR(i2c_multi_inst_acpi_ids),
+	},
+	.probe_new = i2c_multi_inst_probe,
+	.remove = i2c_multi_inst_remove,
+};
+
+module_i2c_driver(i2c_multi_inst_driver);
+
+MODULE_DESCRIPTION("I2C multi instantiate pseudo device driver");
+MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
+MODULE_LICENSE("GPL");