| Message ID | 20221124110718.3925934-2-sbinding@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Use ACPI_COMPANION macro to obtain acpi_device in cs35l41_hda | expand |
On Thu, Nov 24, 2022 at 1:07 PM Stefan Binding <sbinding@opensource.cirrus.com> wrote: > > This allows the i2c driver to obtain the ACPI_COMPANION. As far as I get how it's done in the SPI case the real fix should lie among i2c_acpi_new_device_by_fwnode(), right?
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@opensource.cirrus.com> > --- > 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 . 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
Hi, On 11/24/22 12:35, Andy Shevchenko wrote: > On Thu, Nov 24, 2022 at 1:07 PM Stefan Binding > <sbinding@opensource.cirrus.com> wrote: >> >> This allows the i2c driver to obtain the ACPI_COMPANION. > > As far as I get how it's done in the SPI case the real fix should lie > among i2c_acpi_new_device_by_fwnode(), right? Eventually maybe, but not for the initial change. It is complicated, making this change has side-effects and we want to limit those side-effects to only i2c-clients instantiated from serial-multi-instantiate for now, see my other reply to this patch. I do believe that we eventually want to make this change, to easily give drivers access to all sorts of info (e.g. _DSM methods) from the matching ACPI fw-node, but as I said it is complicated... Regards, Hans
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@opensource.cirrus.com> >> --- >> 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
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)
This allows the i2c driver to obtain the ACPI_COMPANION. Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> --- drivers/platform/x86/serial-multi-instantiate.c | 1 + 1 file changed, 1 insertion(+)