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

Message ID 20180701122343.27194-3-hdegoede@redhat.com
State Rejected
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
Mika Westerberg Aug. 1, 2018, 8:13 a.m. | #3
Hi,

On Sun, Jul 01, 2018 at 02:23:43PM +0200, Hans de Goede wrote:
> +static const struct acpi_device_id i2c_multi_inst_acpi_ids[] = {
> +	{ "BSG1160", (unsigned long)bsg1160_data },
> +	{ }
> +};

This seems to affect only these combo devices with accelerometer and
some other sensor in a single package or so. I wonder if it makes more
sense to handle this in a driver for BSG1160 and leave I2C/ACPI core
untouched?
Hans de Goede Aug. 1, 2018, 8:43 a.m. | #4
Hi,

On 01-08-18 10:13, Mika Westerberg wrote:
> Hi,
> 
> On Sun, Jul 01, 2018 at 02:23:43PM +0200, Hans de Goede wrote:
>> +static const struct acpi_device_id i2c_multi_inst_acpi_ids[] = {
>> +	{ "BSG1160", (unsigned long)bsg1160_data },
>> +	{ }
>> +};
> 
> This seems to affect only these combo devices with accelerometer and
> some other sensor in a single package or so. I wonder if it makes more
> sense to handle this in a driver for BSG1160 and leave I2C/ACPI core
> untouched?

That would require injecting a whole lot of platform specific code
into the platform agnostic drivers/iio/accel/bmc150-accel-i2c.c
driver, something which I've tried before and has pretty much
been nacked.

Heikki has another use-case with a single ACPI fwnode describing
multiple independent i2c clients.

The existing drivers/platform/x86/intel_cht_int33fe.c is yet another
case of this, except that in that case we don't care about the
i2c_client described by the 1st ACPI i2c resource, so it just binds
directly to that i2c-client as it is otherwise unused.

IOW there is a pattern here and I would like to come up with
a fix for the pattern, rather then adding ugly hacks to various
otherwise platform agnostic drivers.

Regards,

Hans
Mika Westerberg Aug. 1, 2018, 9:41 a.m. | #5
On Wed, Aug 01, 2018 at 10:43:14AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 01-08-18 10:13, Mika Westerberg wrote:
> > Hi,
> > 
> > On Sun, Jul 01, 2018 at 02:23:43PM +0200, Hans de Goede wrote:
> > > +static const struct acpi_device_id i2c_multi_inst_acpi_ids[] = {
> > > +	{ "BSG1160", (unsigned long)bsg1160_data },
> > > +	{ }
> > > +};
> > 
> > This seems to affect only these combo devices with accelerometer and
> > some other sensor in a single package or so. I wonder if it makes more
> > sense to handle this in a driver for BSG1160 and leave I2C/ACPI core
> > untouched?
> 
> That would require injecting a whole lot of platform specific code
> into the platform agnostic drivers/iio/accel/bmc150-accel-i2c.c
> driver, something which I've tried before and has pretty much
> been nacked.
> 
> Heikki has another use-case with a single ACPI fwnode describing
> multiple independent i2c clients.
> 
> The existing drivers/platform/x86/intel_cht_int33fe.c is yet another
> case of this, except that in that case we don't care about the
> i2c_client described by the 1st ACPI i2c resource, so it just binds
> directly to that i2c-client as it is otherwise unused.

I see. Thanks for the explanation.

> IOW there is a pattern here and I would like to come up with
> a fix for the pattern, rather then adding ugly hacks to various
> otherwise platform agnostic drivers.

I agree.

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