| Message ID | 20190521150502.27305-5-ckeepax@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | I2C IRQ Probe Improvements | expand |
On Tue, May 21, 2019 at 04:05:01PM +0100, Charles Keepax wrote: > It makes sense to contain all the ACPI IRQ handling in a single helper > function. > Note that this one is somewhat interesting, it seems the search > through the resource list is done against the companion device > of the adapter but the GPIO search is done against the companion > device of the client. It feels to me like these really should > be done on the same device, and certainly this is what SPI > does (both against the equivalent of the adapter). Perhaps > someone with more ACPI knowledge than myself could comment? It would be interesting to see the path how you come to this conclusion. > acpi_dev_free_resource_list(&resource_list); > > + if (*irq < 0) > + *irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(&client->dev), 0); I think adev here is what we may use here. You may put assert here and see if it happens when you test your series.
On Tue, May 21, 2019 at 08:27:26PM +0300, Andy Shevchenko wrote: > On Tue, May 21, 2019 at 04:05:01PM +0100, Charles Keepax wrote: > > It makes sense to contain all the ACPI IRQ handling in a single helper > > function. > > > Note that this one is somewhat interesting, it seems the search > > through the resource list is done against the companion device > > of the adapter but the GPIO search is done against the companion > > device of the client. It feels to me like these really should > > be done on the same device, and certainly this is what SPI > > does (both against the equivalent of the adapter). Perhaps > > someone with more ACPI knowledge than myself could comment? > > It would be interesting to see the path how you come to this conclusion. > Apologies but I am not sure which conclusion you are referencing. Assuming it is them being called with different acpi_device's. It is perhaps me misunderstanding things but it looks like i2c_acpi_get_info implies the adev should correspond to the adapter. Where as acpi_dev_gpio_irq_get is called with the result of ACPI_COMPANION(dev) where dev is client->dev. > > acpi_dev_free_resource_list(&resource_list); > > > > + if (*irq < 0) > > + *irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(&client->dev), 0); > > I think adev here is what we may use here. > Indeed that is what I would have expected as well, I will update the code to do so and hopefully any issues will come out in testing. > You may put assert here and see if it happens when you test your series. > Alas I don't have a good way to test this series, they come out of some additional work Wolfram wanted based on some issues caused by a device tree fix I made a while back. Thanks, Charles
diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c index e332760bf9ebc..0c882d956e9a4 100644 --- a/drivers/i2c/i2c-core-acpi.c +++ b/drivers/i2c/i2c-core-acpi.c @@ -164,6 +164,9 @@ int i2c_acpi_get_irq(struct i2c_client *client, int *irq) acpi_dev_free_resource_list(&resource_list); + if (*irq < 0) + *irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(&client->dev), 0); + return 0; } diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index c1afa17a76bfc..f958b50c78c04 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -336,10 +336,6 @@ static int i2c_device_probe(struct device *dev) irq = of_irq_get(dev->of_node, 0); } else if (ACPI_COMPANION(dev)) { i2c_acpi_get_irq(client, &irq); - - if (irq == -ENOENT) - irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(dev), - 0); } if (irq == -EPROBE_DEFER) return irq;
It makes sense to contain all the ACPI IRQ handling in a single helper function. Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> --- Note that this one is somewhat interesting, it seems the search through the resource list is done against the companion device of the adapter but the GPIO search is done against the companion device of the client. It feels to me like these really should be done on the same device, and certainly this is what SPI does (both against the equivalent of the adapter). Perhaps someone with more ACPI knowledge than myself could comment? Thanks, Charles drivers/i2c/i2c-core-acpi.c | 3 +++ drivers/i2c/i2c-core-base.c | 4 ---- 2 files changed, 3 insertions(+), 4 deletions(-)