This allows the i2c driver to obtain the ACPI_COMPANION.
Signed-off-by: Stefan Binding <[email protected]>
---
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)
--
2.34.1
On Thu, Nov 24, 2022 at 1:07 PM Stefan Binding
<[email protected]> 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?
--
With Best Regards,
Andy Shevchenko
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 <[email protected]>
> ---
> 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
> <[email protected]> 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 <[email protected]>
>> ---
>> 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