Hi,
On 09-08-18 13:09, Heikki Krogerus wrote:
On Thu, Aug 09, 2018 at 11:15:58AM +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 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@xxxxxxxxxx>
---
<snip>
+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->num_clients = i;
+
+ for (i = 0; i < multi->num_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;
This seems to assume that we always have GpioInt with assigned for
these devices, but that's wrong. This needs to work with normal
Interrupt type resources as well.
The TI USB PD controller instances (HID 3515) for exmaple have normal
Interrupts assigned to them
Why not use the "irq_idx" with normal interrupts, and add a new member
for the GpioInts, something like gpio_irq_idx?
I don't have any use-cases to test using normal interrupts, so I
will leave adding support for that to the first patch adding support
for a device which needs that, such as the HID3515 you mention.
I do agree that it is good to prepare for adding this later, so
I'm going to post a v6 doing s/irq_idx/gpio_irq_idx/
Regards,
Hans
+ }
+ 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;
+}
Thanks,
