This series attempts to align as much IRQ handling into the
probe path as possible. Note that I don't have a great setup
for testing these patches so they are mostly just build tested
and need careful review and testing before any of them are
merged.
The series brings the ACPI path inline with the way the device
tree path handles the IRQ entirely at probe time. However,
it still leaves any IRQ specified through the board_info as
being handled at device time. In that case we need to cache
something from the board_info until probe time, which leaves
any alternative solution with something basically the same as
the current handling although perhaps caching more stuff.
Thanks,
Charles
See previous discussions:
- https://lkml.org/lkml/2019/2/15/989
- https://www.spinics.net/lists/linux-i2c/msg39541.html
Charles Keepax (7):
i2c: core: Allow whole core to use i2c_dev_irq_from_resources
i2c: acpi: Use available IRQ helper functions
i2c: acpi: Factor out getting the IRQ from ACPI
i2c: core: Make i2c_acpi_get_irq available to the rest of the I2C core
i2c: core: Move ACPI IRQ handling to probe time
i2c: core: Move ACPI gpio IRQ handling into i2c_acpi_get_irq
i2c: core: Tidy up handling of init_irq
drivers/i2c/i2c-core-acpi.c | 57 +++++++++++++++++++++++++++++++--------------
drivers/i2c/i2c-core-base.c | 11 +++++----
drivers/i2c/i2c-core.h | 11 +++++++++
3 files changed, 57 insertions(+), 22 deletions(-)
--
2.11.0
Only set init_irq during i2c_device_new and only handle client->irq on
the probe/remove paths.
Suggested-by: Benjamin Tissoires <[email protected]>
Reviewed-by: Mika Westerberg <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Charles Keepax <[email protected]>
---
No changes since v4.
Thanks,
Charles
drivers/i2c/i2c-core-base.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 95a0380286c1c..a3dbaefe4ca66 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -314,6 +314,8 @@ static int i2c_device_probe(struct device *dev)
driver = to_i2c_driver(dev->driver);
+ client->irq = client->init_irq;
+
if (!client->irq && !driver->disable_i2c_core_irq_mapping) {
int irq = -ENOENT;
@@ -424,7 +426,7 @@ static int i2c_device_remove(struct device *dev)
dev_pm_clear_wake_irq(&client->dev);
device_init_wakeup(&client->dev, false);
- client->irq = client->init_irq;
+ client->irq = 0;
if (client->flags & I2C_CLIENT_HOST_NOTIFY)
pm_runtime_put(&client->adapter->dev);
@@ -741,7 +743,6 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf
if (!client->init_irq)
client->init_irq = i2c_dev_irq_from_resources(info->resources,
info->num_resources);
- client->irq = client->init_irq;
strlcpy(client->name, info->type, sizeof(client->name));
--
2.11.0
It makes sense to contain all the ACPI IRQ handling in a single helper
function.
Reviewed-by: Mika Westerberg <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Charles Keepax <[email protected]>
---
No changes since v4.
Thanks,
Charles
drivers/i2c/i2c-core-acpi.c | 3 +++
drivers/i2c/i2c-core-base.c | 3 ---
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index f752879772f64..e21e31d661ba7 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -167,6 +167,9 @@ int i2c_acpi_get_irq(struct acpi_device *adev)
acpi_dev_free_resource_list(&resource_list);
+ if (irq == -ENOENT)
+ irq = acpi_dev_gpio_irq_get(adev, 0);
+
return irq;
}
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 060d1a3a7ea4c..95a0380286c1c 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -328,9 +328,6 @@ static int i2c_device_probe(struct device *dev)
irq = of_irq_get(dev->of_node, 0);
} else if (ACPI_COMPANION(dev)) {
irq = i2c_acpi_get_irq(ACPI_COMPANION(dev));
-
- if (irq == -ENOENT)
- irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(dev), 0);
}
if (irq == -EPROBE_DEFER)
return irq;
--
2.11.0
Bring the ACPI path in sync with the device tree path and handle all the
IRQ fetching at probe time. This leaves the only IRQ handling at device
registration time being that which is passed directly through the board
info as either a resource or an actual IRQ number.
Reviewed-by: Mika Westerberg <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Charles Keepax <[email protected]>
---
Changes since v4:
- Pass acpi_device to i2c_acpi_get_irq
- Pass the client acpi_device rather than the adaptor, I think (maybe
hope is more accurate) this should fix the issue seen by Benjamin.
Thanks,
Charles
drivers/i2c/i2c-core-acpi.c | 5 -----
drivers/i2c/i2c-core-base.c | 5 ++++-
2 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index 0ddfcca6091e1..f752879772f64 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -205,11 +205,6 @@ static int i2c_acpi_get_info(struct acpi_device *adev,
if (adapter_handle)
*adapter_handle = lookup.adapter_handle;
- /* Then fill IRQ number if any */
- ret = i2c_acpi_get_irq(adev);
- if (ret > 0)
- info->irq = ret;
-
acpi_set_modalias(adev, dev_name(&adev->dev), info->type,
sizeof(info->type));
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 8a303246d534b..060d1a3a7ea4c 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -327,7 +327,10 @@ static int i2c_device_probe(struct device *dev)
if (irq == -EINVAL || irq == -ENODATA)
irq = of_irq_get(dev->of_node, 0);
} else if (ACPI_COMPANION(dev)) {
- irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(dev), 0);
+ irq = i2c_acpi_get_irq(ACPI_COMPANION(dev));
+
+ if (irq == -ENOENT)
+ irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(dev), 0);
}
if (irq == -EPROBE_DEFER)
return irq;
--
2.11.0
Use the available IRQ helper functions, most of the functions have
additional helpful side affects like configuring the trigger type of the
IRQ.
Reviewed-by: Mika Westerberg <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Charles Keepax <[email protected]>
---
No changes since v4.
Thanks,
Charles
drivers/i2c/i2c-core-acpi.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index f1d648962b223..47d5b1c5ec9e0 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -133,14 +133,25 @@ static int i2c_acpi_do_lookup(struct acpi_device *adev,
return 0;
}
+static int i2c_acpi_add_resource(struct acpi_resource *ares, void *data)
+{
+ int *irq = data;
+ struct resource r;
+
+ if (*irq <= 0 && acpi_dev_resource_interrupt(ares, 0, &r))
+ *irq = i2c_dev_irq_from_resources(&r, 1);
+
+ return 1; /* No need to add resource to the list */
+}
+
static int i2c_acpi_get_info(struct acpi_device *adev,
struct i2c_board_info *info,
struct i2c_adapter *adapter,
acpi_handle *adapter_handle)
{
struct list_head resource_list;
- struct resource_entry *entry;
struct i2c_acpi_lookup lookup;
+ int irq = -ENOENT;
int ret;
memset(&lookup, 0, sizeof(lookup));
@@ -172,16 +183,13 @@ static int i2c_acpi_get_info(struct acpi_device *adev,
/* Then fill IRQ number if any */
INIT_LIST_HEAD(&resource_list);
- ret = acpi_dev_get_resources(adev, &resource_list, NULL, NULL);
+ ret = acpi_dev_get_resources(adev, &resource_list,
+ i2c_acpi_add_resource, &irq);
if (ret < 0)
return -EINVAL;
- resource_list_for_each_entry(entry, &resource_list) {
- if (resource_type(entry->res) == IORESOURCE_IRQ) {
- info->irq = entry->res->start;
- break;
- }
- }
+ if (irq > 0)
+ info->irq = irq;
acpi_dev_free_resource_list(&resource_list);
--
2.11.0
On Thu, Jun 20, 2019 at 02:34:15PM +0100, Charles Keepax wrote:
> Use the available IRQ helper functions, most of the functions have
> additional helpful side affects like configuring the trigger type of the
> IRQ.
>
> Reviewed-by: Mika Westerberg <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Charles Keepax <[email protected]>
Some last minute observations / questions.
> + struct resource r;
> +
> + if (*irq <= 0 && acpi_dev_resource_interrupt(ares, 0, &r))
> + *irq = i2c_dev_irq_from_resources(&r, 1);
> +
> + return 1; /* No need to add resource to the list */
If we don't add it to the list, do we still need to manage the empty
resource_list below?
> /* Then fill IRQ number if any */
> INIT_LIST_HEAD(&resource_list);
> - ret = acpi_dev_get_resources(adev, &resource_list, NULL, NULL);
> + ret = acpi_dev_get_resources(adev, &resource_list,
> + i2c_acpi_add_resource, &irq);
> if (ret < 0)
> return -EINVAL;
>
> - resource_list_for_each_entry(entry, &resource_list) {
> - if (resource_type(entry->res) == IORESOURCE_IRQ) {
> - info->irq = entry->res->start;
> - break;
> - }
> - }
> + if (irq > 0)
> + info->irq = irq;
Hmm... can't we just assign it directly inside the _add_resource() call back as
original code did?
--
With Best Regards,
Andy Shevchenko
On Thu, Jun 20, 2019 at 02:34:13PM +0100, Charles Keepax wrote:
> This series attempts to align as much IRQ handling into the
> probe path as possible. Note that I don't have a great setup
> for testing these patches so they are mostly just build tested
> and need careful review and testing before any of them are
> merged.
>
> The series brings the ACPI path inline with the way the device
> tree path handles the IRQ entirely at probe time. However,
> it still leaves any IRQ specified through the board_info as
> being handled at device time. In that case we need to cache
> something from the board_info until probe time, which leaves
> any alternative solution with something basically the same as
> the current handling although perhaps caching more stuff.
Thank you for an update.
I asked few last minute questions on per patch basis.
>
> Thanks,
> Charles
>
> See previous discussions:
> - https://lkml.org/lkml/2019/2/15/989
> - https://www.spinics.net/lists/linux-i2c/msg39541.html
>
> Charles Keepax (7):
> i2c: core: Allow whole core to use i2c_dev_irq_from_resources
> i2c: acpi: Use available IRQ helper functions
> i2c: acpi: Factor out getting the IRQ from ACPI
> i2c: core: Make i2c_acpi_get_irq available to the rest of the I2C core
> i2c: core: Move ACPI IRQ handling to probe time
> i2c: core: Move ACPI gpio IRQ handling into i2c_acpi_get_irq
> i2c: core: Tidy up handling of init_irq
>
> drivers/i2c/i2c-core-acpi.c | 57 +++++++++++++++++++++++++++++++--------------
> drivers/i2c/i2c-core-base.c | 11 +++++----
> drivers/i2c/i2c-core.h | 11 +++++++++
> 3 files changed, 57 insertions(+), 22 deletions(-)
>
> --
> 2.11.0
>
--
With Best Regards,
Andy Shevchenko
On Thu, Jun 20, 2019 at 05:52:21PM +0300, Andy Shevchenko wrote:
> On Thu, Jun 20, 2019 at 02:34:15PM +0100, Charles Keepax wrote:
> > Use the available IRQ helper functions, most of the functions have
> > additional helpful side affects like configuring the trigger type of the
> > IRQ.
> >
> > Reviewed-by: Mika Westerberg <[email protected]>
> > Reviewed-by: Andy Shevchenko <[email protected]>
> > Signed-off-by: Charles Keepax <[email protected]>
>
> Some last minute observations / questions.
>
> > + struct resource r;
> > +
> > + if (*irq <= 0 && acpi_dev_resource_interrupt(ares, 0, &r))
> > + *irq = i2c_dev_irq_from_resources(&r, 1);
> > +
> > + return 1; /* No need to add resource to the list */
>
> If we don't add it to the list, do we still need to manage the empty
> resource_list below?
>
I think you are right looking closely I think we can skip this. I
might update the comment here to make it clear the list needs
freed if this is changed though.
> > /* Then fill IRQ number if any */
> > INIT_LIST_HEAD(&resource_list);
> > - ret = acpi_dev_get_resources(adev, &resource_list, NULL, NULL);
> > + ret = acpi_dev_get_resources(adev, &resource_list,
> > + i2c_acpi_add_resource, &irq);
> > if (ret < 0)
> > return -EINVAL;
> >
> > - resource_list_for_each_entry(entry, &resource_list) {
> > - if (resource_type(entry->res) == IORESOURCE_IRQ) {
> > - info->irq = entry->res->start;
> > - break;
> > - }
> > - }
>
> > + if (irq > 0)
> > + info->irq = irq;
>
> Hmm... can't we just assign it directly inside the _add_resource() call back as
> original code did?
>
Again I think you are correct here, my thinking had been to
preserve the original functionality of only overwriting the value
in info->irq if we found one. But it looks like all callers don't
pass anything meaningful in this field so changing that behaviour
shouldn't matter.
Thanks,
Charles