Hi, On 11/24/22 12:47, Hans de Goede wrote: > Hi Stefan, > > On 11/24/22 12:07, Stefan Binding wrote: >> This allows the i2c driver to obtain the ACPI_COMPANION. >> >> Signed-off-by: Stefan Binding <sbinding@xxxxxxxxxxxxxxxxxxxxx> >> --- >> drivers/platform/x86/serial-multi-instantiate.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/platform/x86/serial-multi-instantiate.c b/drivers/platform/x86/serial-multi-instantiate.c >> index 5362f1a7b77c..15ef2f3c442e 100644 >> --- a/drivers/platform/x86/serial-multi-instantiate.c >> +++ b/drivers/platform/x86/serial-multi-instantiate.c >> @@ -194,6 +194,7 @@ static int smi_i2c_probe(struct platform_device *pdev, struct smi *smi, >> strscpy(board_info.type, inst_array[i].type, I2C_NAME_SIZE); >> snprintf(name, sizeof(name), "%s-%s.%d", dev_name(dev), inst_array[i].type, i); >> board_info.dev_name = name; >> + board_info.fwnode = acpi_fwnode_handle(adev); >> >> ret = smi_get_irq(pdev, adev, &inst_array[i]); >> if (ret < 0) > > I'm afraid that making this change is not as straight forward as it looks. > > I know that I have tried to do this in the past and it failed. > > IIRC there were 3 problems: > > 1. I was expecting this to also allow the driver for the instantiated > i2c-client to be able to bind using an acpi_match_table but that > unfortunately does not work. acpi_match_table matches only work for > the first physical_node linked under > /sys/bus/acpi/devices/xxxx:xx/physical_node and that is the platform > device to which serial-multi-instantiate.c binds. The i2c_client becomes > the second physical node. Note this is not really an issue, > just something to be aware of. > > > 2. This causes the i2c-core to use the first IRQ resource in the ACPI > fwnode as client->irq for any clients for which we do not set an > IRQ when instantiating. Which may very well be wrong. Sometimes that > IRQ is only valid for the first i2c-client which we instantiate; and > not for the others! And sometimes it is a problem because it may > point to an irqchip for which we never wrote a driver leading to > all probes of the i2c-client failing with -EPROBE_DEFER, see: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d1d84bb95364ed604015c2b788caaf3dbca0262f > > Note that patch has been reverted since that specific -EPROBE_DEFER > issue has been solved by making the ACPI core instantiate a > platform_device instead of an i2c_client (in this case we > did not need the actual i2c_client at all). > > The current i2c-core code has a (!client-irq) test guarding its > code of trying to use the first ACPI fwnode IRQ resource. > > So we could disable this by setting client->irq = -ENOENT in > serial-multi-instantiate.c when (inst->flags & IRQ_RESOURCE_TYPE) == > IRQ_RESOURCE_NONE). But that will introduce a new problem. Many > i2c-drivers check if there is an IRQ for them to use by doing: > "if (client->irq) request_irq(client->irq, ...)" but then with > error checking/so setting client->irq to -ENOENT will cause > the request_irq to fail, leading the probe to fail. > > So before you can write a patch setting client->irq = -ENOENT > when (inst->flags & IRQ_RESOURCE_TYPE) == IRQ_RESOURCE_NONE), > you would first need to patch all i2c-drivers for clients > instantiated through serial-multi-instantiate.c changing: > > if (client->irq) { > ... > } > > to: > > if (client->irq > 0) { > ... > } > > Note this is not as bad as it sounds, since there are only > a few drivers for clients instantiated by serial-multi-instantiate.c . Possibly a nicer way to fix this would be to make the i2c-core change client->irq to 0 if it is -ENOENT before calling the i2c_driver's probe method, thus fixing things centrally for all i2c-drivers without needing to audit/patch them all. Specifically in: drivers/i2c/i2c-core-base.c: i2c_device_probe() change: if (!client->irq) { ... } to: if (!client->irq) { ... } else if (client->irq == -ENOENT) { client->irq = 0; /* Drivers expect 0 for "no-IRQ" */ } And maybe as Andy suggested, handle at least the IRQ in i2c_acpi_new_device_by_fwnode() by adding something like that there: /* Disable the i2c-core attempting to get an IRQ from ACPI itself */ if (!board_info->irq) board_info->irq= -ENOENT; I also agree with Andy that setting board_info->fw_node would be done there ideally too. But then you would need to extend the audit of impacted drivers mentioned below to also include drivers for i2c-clients instantiated through other code-paths calling i2c_acpi_new_device_by_fwnode() (of which there are not many, but there are a few others). > 3. Some drivers may check for an ACPI companion device and then > change their behavior. So all drivers for clients instantiated > through serial-multi-instantiate.c will need to be audited for > this (and a summary of this audit needs to be added to the commit > msg). Regards, Hans
