| Message ID | 20180701122343.27194-3-hdegoede@redhat.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | i2c: ACPI: Add multi-instantiate pseudo driver | expand |
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.
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
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?
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
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.
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");
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