* [PATCH v8 01/10] ACPI: scan: Remove the second DSDT traversal
2023-03-29 10:09 [PATCH v8 00/10] ACPI _CRS CSI-2 and MIPI DisCo for Imaging support Sakari Ailus
@ 2023-03-29 10:09 ` Sakari Ailus
2023-05-09 18:06 ` Rafael J. Wysocki
2023-03-29 10:09 ` [PATCH v8 02/10] ACPI: property: Parse data node string references in properties Sakari Ailus
` (8 subsequent siblings)
9 siblings, 1 reply; 36+ messages in thread
From: Sakari Ailus @ 2023-03-29 10:09 UTC (permalink / raw)
To: linux-acpi; +Cc: linux-media, rafael, andriy.shevchenko, heikki.krogerus
Collect the devices with _DEP into a list and continue processing them
after a full traversal, instead of doing a full second traversal of the
tree.
This makes the second DSDT traversal pass unnecessary as we already have
the nodes we're interested in in a linked list.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/acpi/scan.c | 125 ++++++++++++++++++++++++++++++++------------
1 file changed, 92 insertions(+), 33 deletions(-)
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 0c6f06abe3f4..280d12e0aa2f 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -2029,10 +2029,52 @@ static u32 acpi_scan_check_dep(acpi_handle handle, bool check_dep)
return count;
}
-static bool acpi_bus_scan_second_pass;
+/**
+ * struct acpi_postponed_handle - A postponed ACPI handle
+ * @list: Entry in a postponed list
+ * @handle: The postponed handle
+ *
+ * One such entry represents an ACPI handle the scanning of which has been
+ * postponed.
+ */
+struct acpi_postponed_handle {
+ struct list_head list;
+ acpi_handle handle;
+};
+
+/**
+ * struct acpi_scan_context - Context for scanning ACPI devices
+ * @postponed_head: The list head of the postponed ACPI handles
+ * @device: The first encountered device, typically the root of the scanned tree
+ */
+struct acpi_scan_context {
+ struct list_head postponed_head;
+ struct acpi_device *device;
+};
+
+/**
+ * acpi_bus_handle_postpone - Add an ACPI handle to a given postponed list
+ * @handle: The ACPI handle
+ * @head: Postponed list head
+ *
+ * Add a given ACPI handle to a list of ACPI objects for which the creation
+ * of the device objects is to be postponed.
+ */
+static void acpi_bus_handle_postpone(acpi_handle handle,
+ struct list_head *head)
+{
+ struct acpi_postponed_handle *ph;
+
+ ph = kzalloc(sizeof(*ph), GFP_KERNEL);
+ if (!ph)
+ return;
+
+ ph->handle = handle;
+ list_add(&ph->list, head);
+}
static acpi_status acpi_bus_check_add(acpi_handle handle, bool check_dep,
- struct acpi_device **adev_p)
+ struct acpi_scan_context *ctx)
{
struct acpi_device *device = acpi_fetch_acpi_dev(handle);
acpi_object_type acpi_type;
@@ -2051,7 +2093,7 @@ static acpi_status acpi_bus_check_add(acpi_handle handle, bool check_dep,
/* Bail out if there are dependencies. */
if (acpi_scan_check_dep(handle, check_dep) > 0) {
- acpi_bus_scan_second_pass = true;
+ acpi_bus_handle_postpone(handle, &ctx->postponed_head);
return AE_CTRL_DEPTH;
}
@@ -2086,22 +2128,22 @@ static acpi_status acpi_bus_check_add(acpi_handle handle, bool check_dep,
acpi_scan_init_hotplug(device);
out:
- if (!*adev_p)
- *adev_p = device;
+ if (!ctx->device)
+ ctx->device = device;
return AE_OK;
}
static acpi_status acpi_bus_check_add_1(acpi_handle handle, u32 lvl_not_used,
- void *not_used, void **ret_p)
+ void *ctx, void **unused)
{
- return acpi_bus_check_add(handle, true, (struct acpi_device **)ret_p);
+ return acpi_bus_check_add(handle, true, (struct acpi_scan_context *)ctx);
}
static acpi_status acpi_bus_check_add_2(acpi_handle handle, u32 lvl_not_used,
- void *not_used, void **ret_p)
+ void *ctx, void **device)
{
- return acpi_bus_check_add(handle, false, (struct acpi_device **)ret_p);
+ return acpi_bus_check_add(handle, false, (struct acpi_scan_context *)ctx);
}
static void acpi_default_enumeration(struct acpi_device *device)
@@ -2422,37 +2464,54 @@ EXPORT_SYMBOL_GPL(acpi_dev_get_next_consumer_dev);
*/
int acpi_bus_scan(acpi_handle handle)
{
- struct acpi_device *device = NULL;
-
- acpi_bus_scan_second_pass = false;
-
- /* Pass 1: Avoid enumerating devices with missing dependencies. */
+ struct acpi_scan_context ctx = {
+ .postponed_head = LIST_HEAD_INIT(ctx.postponed_head),
+ };
+ struct acpi_postponed_handle *ph, *tmp_ph;
+ int ret = 0;
- if (ACPI_SUCCESS(acpi_bus_check_add(handle, true, &device)))
+ if (ACPI_SUCCESS(acpi_bus_check_add(handle, true, &ctx)))
acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
- acpi_bus_check_add_1, NULL, NULL,
- (void **)&device);
-
- if (!device)
- return -ENODEV;
-
- acpi_bus_attach(device, (void *)true);
+ acpi_bus_check_add_1, NULL, (void *)&ctx,
+ NULL);
- if (!acpi_bus_scan_second_pass)
- return 0;
-
- /* Pass 2: Enumerate all of the remaining devices. */
+ if (!ctx.device) {
+ ret = -ENODEV;
+ goto out_release;
+ }
- device = NULL;
+ acpi_bus_attach(ctx.device, (void *)true);
- if (ACPI_SUCCESS(acpi_bus_check_add(handle, false, &device)))
- acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
- acpi_bus_check_add_2, NULL, NULL,
- (void **)&device);
+ /*
+ * Proceed to register ACPI devices that were postponed due to _DEP
+ * objects during the namespace walk.
+ */
+ list_for_each_entry_safe(ph, tmp_ph, &ctx.postponed_head, list) {
+ list_del(&ph->list);
+ /* Set device NULL here to obtain the root for this sub-tree. */
+ ctx.device = NULL;
+ /*
+ * Do this manually, as the namespace walk will only include
+ * sub-nodes, not the node itself. ctx.device is set to the
+ * ACPI device corresponding ph->handle.
+ */
+ acpi_bus_check_add_2(ph->handle, 0, &ctx, NULL);
+ /* Walk the rest of the sub-namespace. */
+ acpi_walk_namespace(ACPI_TYPE_ANY, ph->handle, ACPI_UINT32_MAX,
+ acpi_bus_check_add_2, NULL, (void *)&ctx,
+ NULL);
+ if (ctx.device)
+ acpi_bus_attach(ctx.device, NULL);
+ kfree(ph);
+ }
- acpi_bus_attach(device, NULL);
+out_release:
+ list_for_each_entry_safe(ph, tmp_ph, &ctx.postponed_head, list) {
+ list_del(&ph->list);
+ kfree(ph);
+ }
- return 0;
+ return ret;
}
EXPORT_SYMBOL(acpi_bus_scan);
--
2.30.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v8 01/10] ACPI: scan: Remove the second DSDT traversal
2023-03-29 10:09 ` [PATCH v8 01/10] ACPI: scan: Remove the second DSDT traversal Sakari Ailus
@ 2023-05-09 18:06 ` Rafael J. Wysocki
2023-05-09 20:49 ` Sakari Ailus
0 siblings, 1 reply; 36+ messages in thread
From: Rafael J. Wysocki @ 2023-05-09 18:06 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-acpi, linux-media, rafael, andriy.shevchenko,
heikki.krogerus
On Wed, Mar 29, 2023 at 12:10 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Collect the devices with _DEP into a list and continue processing them
> after a full traversal, instead of doing a full second traversal of the
> tree.
>
> This makes the second DSDT traversal pass unnecessary as we already have
> the nodes we're interested in in a linked list.
The second traversal of the ACPI namespace (it may not be just the
DSDT at that point to be precise) is not really about _DEP handling.
In fact, the latter has been added on top of it.
It is about the PCI enumeration. Namely, when acpi_pci_root_add()
runs for the PCI host bridge object in the ACPI namespace, the entire
device hierarchy below it is walked and all of the ACPI device objects
corresponding to the PCI devices on the bus are assumed to be present.
This means that all of the ACPI device objects need to be created in
the first walk, without binding any ACPI drivers or scan handlers to
them, and the second walk is to find out what is actually represented
by those objects.
It cannot be eliminated in any simple way.
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/acpi/scan.c | 125 ++++++++++++++++++++++++++++++++------------
> 1 file changed, 92 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 0c6f06abe3f4..280d12e0aa2f 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -2029,10 +2029,52 @@ static u32 acpi_scan_check_dep(acpi_handle handle, bool check_dep)
> return count;
> }
>
> -static bool acpi_bus_scan_second_pass;
> +/**
> + * struct acpi_postponed_handle - A postponed ACPI handle
> + * @list: Entry in a postponed list
> + * @handle: The postponed handle
> + *
> + * One such entry represents an ACPI handle the scanning of which has been
> + * postponed.
> + */
> +struct acpi_postponed_handle {
> + struct list_head list;
> + acpi_handle handle;
> +};
> +
> +/**
> + * struct acpi_scan_context - Context for scanning ACPI devices
> + * @postponed_head: The list head of the postponed ACPI handles
> + * @device: The first encountered device, typically the root of the scanned tree
> + */
> +struct acpi_scan_context {
> + struct list_head postponed_head;
> + struct acpi_device *device;
> +};
> +
> +/**
> + * acpi_bus_handle_postpone - Add an ACPI handle to a given postponed list
> + * @handle: The ACPI handle
> + * @head: Postponed list head
> + *
> + * Add a given ACPI handle to a list of ACPI objects for which the creation
> + * of the device objects is to be postponed.
> + */
> +static void acpi_bus_handle_postpone(acpi_handle handle,
> + struct list_head *head)
> +{
> + struct acpi_postponed_handle *ph;
> +
> + ph = kzalloc(sizeof(*ph), GFP_KERNEL);
> + if (!ph)
> + return;
> +
> + ph->handle = handle;
> + list_add(&ph->list, head);
> +}
>
> static acpi_status acpi_bus_check_add(acpi_handle handle, bool check_dep,
> - struct acpi_device **adev_p)
> + struct acpi_scan_context *ctx)
> {
> struct acpi_device *device = acpi_fetch_acpi_dev(handle);
> acpi_object_type acpi_type;
> @@ -2051,7 +2093,7 @@ static acpi_status acpi_bus_check_add(acpi_handle handle, bool check_dep,
>
> /* Bail out if there are dependencies. */
> if (acpi_scan_check_dep(handle, check_dep) > 0) {
> - acpi_bus_scan_second_pass = true;
> + acpi_bus_handle_postpone(handle, &ctx->postponed_head);
> return AE_CTRL_DEPTH;
> }
>
> @@ -2086,22 +2128,22 @@ static acpi_status acpi_bus_check_add(acpi_handle handle, bool check_dep,
> acpi_scan_init_hotplug(device);
>
> out:
> - if (!*adev_p)
> - *adev_p = device;
> + if (!ctx->device)
> + ctx->device = device;
>
> return AE_OK;
> }
>
> static acpi_status acpi_bus_check_add_1(acpi_handle handle, u32 lvl_not_used,
> - void *not_used, void **ret_p)
> + void *ctx, void **unused)
> {
> - return acpi_bus_check_add(handle, true, (struct acpi_device **)ret_p);
> + return acpi_bus_check_add(handle, true, (struct acpi_scan_context *)ctx);
> }
>
> static acpi_status acpi_bus_check_add_2(acpi_handle handle, u32 lvl_not_used,
> - void *not_used, void **ret_p)
> + void *ctx, void **device)
> {
> - return acpi_bus_check_add(handle, false, (struct acpi_device **)ret_p);
> + return acpi_bus_check_add(handle, false, (struct acpi_scan_context *)ctx);
> }
>
> static void acpi_default_enumeration(struct acpi_device *device)
> @@ -2422,37 +2464,54 @@ EXPORT_SYMBOL_GPL(acpi_dev_get_next_consumer_dev);
> */
> int acpi_bus_scan(acpi_handle handle)
> {
> - struct acpi_device *device = NULL;
> -
> - acpi_bus_scan_second_pass = false;
> -
> - /* Pass 1: Avoid enumerating devices with missing dependencies. */
> + struct acpi_scan_context ctx = {
> + .postponed_head = LIST_HEAD_INIT(ctx.postponed_head),
> + };
> + struct acpi_postponed_handle *ph, *tmp_ph;
> + int ret = 0;
>
> - if (ACPI_SUCCESS(acpi_bus_check_add(handle, true, &device)))
> + if (ACPI_SUCCESS(acpi_bus_check_add(handle, true, &ctx)))
> acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
> - acpi_bus_check_add_1, NULL, NULL,
> - (void **)&device);
> -
> - if (!device)
> - return -ENODEV;
> -
> - acpi_bus_attach(device, (void *)true);
> + acpi_bus_check_add_1, NULL, (void *)&ctx,
> + NULL);
>
> - if (!acpi_bus_scan_second_pass)
> - return 0;
> -
> - /* Pass 2: Enumerate all of the remaining devices. */
> + if (!ctx.device) {
> + ret = -ENODEV;
> + goto out_release;
> + }
>
> - device = NULL;
> + acpi_bus_attach(ctx.device, (void *)true);
>
> - if (ACPI_SUCCESS(acpi_bus_check_add(handle, false, &device)))
> - acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
> - acpi_bus_check_add_2, NULL, NULL,
> - (void **)&device);
> + /*
> + * Proceed to register ACPI devices that were postponed due to _DEP
> + * objects during the namespace walk.
> + */
> + list_for_each_entry_safe(ph, tmp_ph, &ctx.postponed_head, list) {
> + list_del(&ph->list);
> + /* Set device NULL here to obtain the root for this sub-tree. */
> + ctx.device = NULL;
> + /*
> + * Do this manually, as the namespace walk will only include
> + * sub-nodes, not the node itself. ctx.device is set to the
> + * ACPI device corresponding ph->handle.
> + */
> + acpi_bus_check_add_2(ph->handle, 0, &ctx, NULL);
> + /* Walk the rest of the sub-namespace. */
> + acpi_walk_namespace(ACPI_TYPE_ANY, ph->handle, ACPI_UINT32_MAX,
> + acpi_bus_check_add_2, NULL, (void *)&ctx,
> + NULL);
> + if (ctx.device)
> + acpi_bus_attach(ctx.device, NULL);
> + kfree(ph);
> + }
>
> - acpi_bus_attach(device, NULL);
> +out_release:
> + list_for_each_entry_safe(ph, tmp_ph, &ctx.postponed_head, list) {
> + list_del(&ph->list);
> + kfree(ph);
> + }
>
> - return 0;
> + return ret;
> }
> EXPORT_SYMBOL(acpi_bus_scan);
>
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v8 01/10] ACPI: scan: Remove the second DSDT traversal
2023-05-09 18:06 ` Rafael J. Wysocki
@ 2023-05-09 20:49 ` Sakari Ailus
2023-05-11 16:08 ` Rafael J. Wysocki
0 siblings, 1 reply; 36+ messages in thread
From: Sakari Ailus @ 2023-05-09 20:49 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: linux-acpi, linux-media, andriy.shevchenko, heikki.krogerus
Hi Rafael,
Thank you for the review.
On Tue, May 09, 2023 at 08:06:28PM +0200, Rafael J. Wysocki wrote:
> On Wed, Mar 29, 2023 at 12:10 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Collect the devices with _DEP into a list and continue processing them
> > after a full traversal, instead of doing a full second traversal of the
> > tree.
> >
> > This makes the second DSDT traversal pass unnecessary as we already have
> > the nodes we're interested in in a linked list.
>
> The second traversal of the ACPI namespace (it may not be just the
> DSDT at that point to be precise) is not really about _DEP handling.
> In fact, the latter has been added on top of it.
>
> It is about the PCI enumeration. Namely, when acpi_pci_root_add()
> runs for the PCI host bridge object in the ACPI namespace, the entire
> device hierarchy below it is walked and all of the ACPI device objects
> corresponding to the PCI devices on the bus are assumed to be present.
> This means that all of the ACPI device objects need to be created in
> the first walk, without binding any ACPI drivers or scan handlers to
> them, and the second walk is to find out what is actually represented
> by those objects.
>
> It cannot be eliminated in any simple way.
My understanding still remains that this patch does not (or other patches
in this set) change the above. It is just how those nodes are reached:
instead of traversing the entire tree and ignoring the devices that have
already an acpi_device created for them, a linked list of devices of
interest is traversed.
Of course it is possible that I have missed something. The codebase isn't
entirely trivial.
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v8 01/10] ACPI: scan: Remove the second DSDT traversal
2023-05-09 20:49 ` Sakari Ailus
@ 2023-05-11 16:08 ` Rafael J. Wysocki
0 siblings, 0 replies; 36+ messages in thread
From: Rafael J. Wysocki @ 2023-05-11 16:08 UTC (permalink / raw)
To: Sakari Ailus
Cc: Rafael J. Wysocki, linux-acpi, linux-media, andriy.shevchenko,
heikki.krogerus
On Tuesday, May 9, 2023 10:49:21 PM CEST Sakari Ailus wrote:
> Hi Rafael,
>
> Thank you for the review.
>
> On Tue, May 09, 2023 at 08:06:28PM +0200, Rafael J. Wysocki wrote:
> > On Wed, Mar 29, 2023 at 12:10 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Collect the devices with _DEP into a list and continue processing them
> > > after a full traversal, instead of doing a full second traversal of the
> > > tree.
> > >
> > > This makes the second DSDT traversal pass unnecessary as we already have
> > > the nodes we're interested in in a linked list.
> >
> > The second traversal of the ACPI namespace (it may not be just the
> > DSDT at that point to be precise) is not really about _DEP handling.
> > In fact, the latter has been added on top of it.
> >
> > It is about the PCI enumeration. Namely, when acpi_pci_root_add()
> > runs for the PCI host bridge object in the ACPI namespace, the entire
> > device hierarchy below it is walked and all of the ACPI device objects
> > corresponding to the PCI devices on the bus are assumed to be present.
> > This means that all of the ACPI device objects need to be created in
> > the first walk, without binding any ACPI drivers or scan handlers to
> > them, and the second walk is to find out what is actually represented
> > by those objects.
> >
> > It cannot be eliminated in any simple way.
>
> My understanding still remains that this patch does not (or other patches
> in this set) change the above. It is just how those nodes are reached:
> instead of traversing the entire tree and ignoring the devices that have
> already an acpi_device created for them, a linked list of devices of
> interest is traversed.
>
> Of course it is possible that I have missed something. The codebase isn't
> entirely trivial.
You are right and I see what it is about now.
However, the implementation is rather fragile and the list added by the
$subject patch is redundant AFAICS, because all of the objects in it
are also present in acpi_dep_list as consumers (adding an object to
acpi_dep_list as a consumer is necessary for that object to be added to the
new list too). Of course, there may be multiple acpi_dep_list for the same
consumer object, but it is not a fundamental problem.
So overall I'd prefer to do something like the appended (untested) patch
instead.
---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: [PATCH] ACPI: scan: Reduce overhead related to devices with dependencies
Notice that all of the objects for which the acpi_scan_check_dep()
return value is greater than 0 are present in acpi_dep_list as consumers
(there may be multiple entries for one object, but that is not a
problem), so after carrying out the initial ACPI namespace walk in which
devices with dependencies are skipped, acpi_bus_scan() can simply walk
acpi_dep_list and enumerate all of the unique consumer objects from
there and their descendants instead of walking the entire target branch
of the ACPI namespace and looking for device objects that have not been
enumerated yet in it.
Because walking acpi_dep_list is generally less overhead than walking
the entire ACPI namespace, use the observation above to reduce the
system initialization overhead related to ACPI, which is particularly
important on large systems.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/acpi/scan.c | 71 ++++++++++++++++++++++++++++++++++--------------
include/acpi/acpi_bus.h | 2 +
2 files changed, 53 insertions(+), 20 deletions(-)
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -289,6 +289,8 @@ struct acpi_dep_data {
acpi_handle supplier;
acpi_handle consumer;
bool honor_dep;
+ bool met;
+ bool free_when_met;
};
/* Performance Management */
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -2029,8 +2029,6 @@ static u32 acpi_scan_check_dep(acpi_hand
return count;
}
-static bool acpi_bus_scan_second_pass;
-
static acpi_status acpi_bus_check_add(acpi_handle handle, bool check_dep,
struct acpi_device **adev_p)
{
@@ -2050,10 +2048,8 @@ static acpi_status acpi_bus_check_add(ac
return AE_OK;
/* Bail out if there are dependencies. */
- if (acpi_scan_check_dep(handle, check_dep) > 0) {
- acpi_bus_scan_second_pass = true;
+ if (acpi_scan_check_dep(handle, check_dep) > 0)
return AE_CTRL_DEPTH;
- }
fallthrough;
case ACPI_TYPE_ANY: /* for ACPI_ROOT_OBJECT */
@@ -2311,8 +2307,12 @@ static int acpi_scan_clear_dep(struct ac
acpi_dev_put(adev);
}
- list_del(&dep->node);
- kfree(dep);
+ if (dep->free_when_met) {
+ list_del(&dep->node);
+ kfree(dep);
+ } else {
+ dep->met = true;
+ }
return 0;
}
@@ -2406,6 +2406,49 @@ struct acpi_device *acpi_dev_get_next_co
}
EXPORT_SYMBOL_GPL(acpi_dev_get_next_consumer_dev);
+static void acpi_scan_postponed(acpi_handle handle)
+{
+ struct acpi_device *adev = NULL;
+ struct acpi_dep_data *dep, *tmp;
+
+ mutex_lock(&acpi_dep_list_lock);
+
+ list_for_each_entry_safe(dep, tmp, &acpi_dep_list, node) {
+ acpi_handle handle = dep->consumer;
+
+ /*
+ * Even though the lock is released here, tmp is guaranteed to
+ * be valid, because none of the list entries following dep is
+ * marked as "free when met" and so they cannot be deleted.
+ */
+ mutex_unlock(&acpi_dep_list_lock);
+
+ /*
+ * In case there are multiple acpi_dep_list entries with the
+ * same consumer, skip the current entry if the consumer device
+ * object corresponding to it is present already.
+ */
+ if (!acpi_fetch_acpi_dev(handle) &&
+ ACPI_SUCCESS(acpi_bus_check_add(handle, false, &adev))) {
+ acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
+ acpi_bus_check_add_2, NULL, NULL,
+ (void **)&adev);
+ acpi_bus_attach(adev, NULL);
+ }
+
+ mutex_lock(&acpi_dep_list_lock);
+
+ if (dep->met) {
+ list_del(&dep->node);
+ kfree(dep);
+ } else {
+ dep->free_when_met = true;
+ }
+ }
+
+ mutex_unlock(&acpi_dep_list_lock);
+}
+
/**
* acpi_bus_scan - Add ACPI device node objects in a given namespace scope.
* @handle: Root of the namespace scope to scan.
@@ -2424,8 +2467,6 @@ int acpi_bus_scan(acpi_handle handle)
{
struct acpi_device *device = NULL;
- acpi_bus_scan_second_pass = false;
-
/* Pass 1: Avoid enumerating devices with missing dependencies. */
if (ACPI_SUCCESS(acpi_bus_check_add(handle, true, &device)))
@@ -2438,19 +2479,9 @@ int acpi_bus_scan(acpi_handle handle)
acpi_bus_attach(device, (void *)true);
- if (!acpi_bus_scan_second_pass)
- return 0;
-
/* Pass 2: Enumerate all of the remaining devices. */
- device = NULL;
-
- if (ACPI_SUCCESS(acpi_bus_check_add(handle, false, &device)))
- acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
- acpi_bus_check_add_2, NULL, NULL,
- (void **)&device);
-
- acpi_bus_attach(device, NULL);
+ acpi_scan_postponed(handle);
return 0;
}
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v8 02/10] ACPI: property: Parse data node string references in properties
2023-03-29 10:09 [PATCH v8 00/10] ACPI _CRS CSI-2 and MIPI DisCo for Imaging support Sakari Ailus
2023-03-29 10:09 ` [PATCH v8 01/10] ACPI: scan: Remove the second DSDT traversal Sakari Ailus
@ 2023-03-29 10:09 ` Sakari Ailus
2023-05-11 17:04 ` Rafael J. Wysocki
2023-05-12 16:04 ` Rafael J. Wysocki
2023-03-29 10:09 ` [PATCH v8 03/10] ACPI: property: Parse _CRS CSI-2 descriptor Sakari Ailus
` (7 subsequent siblings)
9 siblings, 2 replies; 36+ messages in thread
From: Sakari Ailus @ 2023-03-29 10:09 UTC (permalink / raw)
To: linux-acpi; +Cc: linux-media, rafael, andriy.shevchenko, heikki.krogerus
Add support for parsing property references using strings, besides
reference objects that were previously supported. This allows also
referencing data nodes which was not possible with reference objects.
Also add pr_fmt() macro to prefix printouts.
While at it, update copyright.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/acpi/property.c | 110 ++++++++++++++++++++++++++++++++++------
1 file changed, 94 insertions(+), 16 deletions(-)
diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index b8d9eb9a433e..08831ffba26c 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -2,14 +2,17 @@
/*
* ACPI device specific properties support.
*
- * Copyright (C) 2014, Intel Corporation
+ * Copyright (C) 2014-2023, Intel Corporation
* All rights reserved.
*
* Authors: Mika Westerberg <mika.westerberg@linux.intel.com>
- * Darren Hart <dvhart@linux.intel.com>
- * Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+ * Darren Hart <dvhart@linux.intel.com>
+ * Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+ * Sakari Ailus <sakari.ailus@linux.intel.com>
*/
+#define pr_fmt(fmt) "ACPI: " fmt
+
#include <linux/acpi.h>
#include <linux/device.h>
#include <linux/export.h>
@@ -795,7 +798,8 @@ acpi_fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
static int acpi_get_ref_args(struct fwnode_reference_args *args,
struct fwnode_handle *ref_fwnode,
const union acpi_object **element,
- const union acpi_object *end, size_t num_args)
+ const union acpi_object *end, size_t num_args,
+ bool subnode_string)
{
u32 nargs = 0, i;
@@ -803,13 +807,16 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args,
* Find the referred data extension node under the
* referred device node.
*/
- for (; *element < end && (*element)->type == ACPI_TYPE_STRING;
- (*element)++) {
- const char *child_name = (*element)->string.pointer;
-
- ref_fwnode = acpi_fwnode_get_named_child_node(ref_fwnode, child_name);
- if (!ref_fwnode)
- return -EINVAL;
+ if (subnode_string) {
+ for (; *element < end && (*element)->type == ACPI_TYPE_STRING;
+ (*element)++) {
+ const char *child_name = (*element)->string.pointer;
+
+ ref_fwnode = acpi_fwnode_get_named_child_node(ref_fwnode,
+ child_name);
+ if (!ref_fwnode)
+ return -EINVAL;
+ }
}
/*
@@ -820,7 +827,8 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args,
for (i = 0; (*element) + i < end && i < num_args; i++) {
acpi_object_type type = (*element)[i].type;
- if (type == ACPI_TYPE_LOCAL_REFERENCE)
+ if (type == ACPI_TYPE_LOCAL_REFERENCE ||
+ (!subnode_string && type == ACPI_TYPE_STRING))
break;
if (type == ACPI_TYPE_INTEGER)
@@ -844,6 +852,43 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args,
return 0;
}
+static struct fwnode_handle *
+acpi_parse_string_ref(const struct fwnode_handle *fwnode, const char *refstring)
+{
+ acpi_handle scope, handle;
+ struct acpi_data_node *dn;
+ struct acpi_device *device;
+ acpi_status status;
+
+ if (is_acpi_device_node(fwnode)) {
+ scope = to_acpi_device_node(fwnode)->handle;
+ } else if (is_acpi_data_node(fwnode)) {
+ scope = to_acpi_data_node(fwnode)->handle;
+ } else {
+ pr_debug("bad node type for node %pfw\n", fwnode);
+ return ERR_PTR(-EINVAL);
+ }
+
+ status = acpi_get_handle(scope, refstring, &handle);
+ if (ACPI_FAILURE(status)) {
+ acpi_handle_debug(scope, "can't get handle for %s", refstring);
+ return ERR_PTR(-EINVAL);
+ }
+
+ device = acpi_fetch_acpi_dev(handle);
+ if (device)
+ return acpi_fwnode_handle(device);
+
+ status = acpi_get_data_full(handle, acpi_nondev_subnode_tag,
+ (void **)&dn, NULL);
+ if (ACPI_FAILURE(status) || !dn) {
+ acpi_handle_debug(handle, "can't find subnode");
+ return ERR_PTR(-EINVAL);
+ }
+
+ return &dn->fwnode;
+}
+
/**
* __acpi_node_get_property_reference - returns handle to the referenced object
* @fwnode: Firmware node to get the property from
@@ -886,6 +931,7 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
const union acpi_object *element, *end;
const union acpi_object *obj;
const struct acpi_device_data *data;
+ struct fwnode_handle *ref_fwnode;
struct acpi_device *device;
int ret, idx = 0;
@@ -909,16 +955,29 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
args->fwnode = acpi_fwnode_handle(device);
args->nargs = 0;
+ return 0;
+ case ACPI_TYPE_STRING:
+ if (index)
+ return -ENOENT;
+
+ ref_fwnode = acpi_parse_string_ref(fwnode, obj->string.pointer);
+ if (IS_ERR(ref_fwnode))
+ return PTR_ERR(ref_fwnode);
+
+ args->fwnode = ref_fwnode;
+ args->nargs = 0;
+
return 0;
case ACPI_TYPE_PACKAGE:
/*
* If it is not a single reference, then it is a package of
- * references followed by number of ints as follows:
+ * references, followed by number of ints as follows:
*
* Package () { REF, INT, REF, INT, INT }
*
- * The index argument is then used to determine which reference
- * the caller wants (along with the arguments).
+ * Here, REF may be either a local reference or a string. The
+ * index argument is then used to determine which reference the
+ * caller wants (along with the arguments).
*/
break;
default:
@@ -942,7 +1001,26 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
ret = acpi_get_ref_args(idx == index ? args : NULL,
acpi_fwnode_handle(device),
- &element, end, num_args);
+ &element, end, num_args, true);
+ if (ret < 0)
+ return ret;
+
+ if (idx == index)
+ return 0;
+
+ break;
+ case ACPI_TYPE_STRING:
+ ref_fwnode =
+ acpi_parse_string_ref(fwnode,
+ element->string.pointer);
+ if (IS_ERR(ref_fwnode))
+ return PTR_ERR(ref_fwnode);
+
+ element++;
+
+ ret = acpi_get_ref_args(idx == index ? args : NULL,
+ ref_fwnode, &element, end,
+ num_args, false);
if (ret < 0)
return ret;
--
2.30.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v8 02/10] ACPI: property: Parse data node string references in properties
2023-03-29 10:09 ` [PATCH v8 02/10] ACPI: property: Parse data node string references in properties Sakari Ailus
@ 2023-05-11 17:04 ` Rafael J. Wysocki
2023-05-12 16:04 ` Rafael J. Wysocki
1 sibling, 0 replies; 36+ messages in thread
From: Rafael J. Wysocki @ 2023-05-11 17:04 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-acpi, linux-media, rafael, andriy.shevchenko,
heikki.krogerus
On Wed, Mar 29, 2023 at 12:10 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Add support for parsing property references using strings, besides
> reference objects that were previously supported. This allows also
> referencing data nodes which was not possible with reference objects.
>
> Also add pr_fmt() macro to prefix printouts.
>
> While at it, update copyright.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
This one looks good to me, so formally
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
and please add this tag if you resubmit.
Alternatively, I can just queue it up, because IMV this is an
improvement regardless of the rest of the series.
> ---
> drivers/acpi/property.c | 110 ++++++++++++++++++++++++++++++++++------
> 1 file changed, 94 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index b8d9eb9a433e..08831ffba26c 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -2,14 +2,17 @@
> /*
> * ACPI device specific properties support.
> *
> - * Copyright (C) 2014, Intel Corporation
> + * Copyright (C) 2014-2023, Intel Corporation
> * All rights reserved.
> *
> * Authors: Mika Westerberg <mika.westerberg@linux.intel.com>
> - * Darren Hart <dvhart@linux.intel.com>
> - * Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> + * Darren Hart <dvhart@linux.intel.com>
> + * Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> + * Sakari Ailus <sakari.ailus@linux.intel.com>
> */
>
> +#define pr_fmt(fmt) "ACPI: " fmt
> +
> #include <linux/acpi.h>
> #include <linux/device.h>
> #include <linux/export.h>
> @@ -795,7 +798,8 @@ acpi_fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
> static int acpi_get_ref_args(struct fwnode_reference_args *args,
> struct fwnode_handle *ref_fwnode,
> const union acpi_object **element,
> - const union acpi_object *end, size_t num_args)
> + const union acpi_object *end, size_t num_args,
> + bool subnode_string)
> {
> u32 nargs = 0, i;
>
> @@ -803,13 +807,16 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args,
> * Find the referred data extension node under the
> * referred device node.
> */
> - for (; *element < end && (*element)->type == ACPI_TYPE_STRING;
> - (*element)++) {
> - const char *child_name = (*element)->string.pointer;
> -
> - ref_fwnode = acpi_fwnode_get_named_child_node(ref_fwnode, child_name);
> - if (!ref_fwnode)
> - return -EINVAL;
> + if (subnode_string) {
> + for (; *element < end && (*element)->type == ACPI_TYPE_STRING;
> + (*element)++) {
> + const char *child_name = (*element)->string.pointer;
> +
> + ref_fwnode = acpi_fwnode_get_named_child_node(ref_fwnode,
> + child_name);
> + if (!ref_fwnode)
> + return -EINVAL;
> + }
> }
>
> /*
> @@ -820,7 +827,8 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args,
> for (i = 0; (*element) + i < end && i < num_args; i++) {
> acpi_object_type type = (*element)[i].type;
>
> - if (type == ACPI_TYPE_LOCAL_REFERENCE)
> + if (type == ACPI_TYPE_LOCAL_REFERENCE ||
> + (!subnode_string && type == ACPI_TYPE_STRING))
> break;
>
> if (type == ACPI_TYPE_INTEGER)
> @@ -844,6 +852,43 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args,
> return 0;
> }
>
> +static struct fwnode_handle *
> +acpi_parse_string_ref(const struct fwnode_handle *fwnode, const char *refstring)
> +{
> + acpi_handle scope, handle;
> + struct acpi_data_node *dn;
> + struct acpi_device *device;
> + acpi_status status;
> +
> + if (is_acpi_device_node(fwnode)) {
> + scope = to_acpi_device_node(fwnode)->handle;
> + } else if (is_acpi_data_node(fwnode)) {
> + scope = to_acpi_data_node(fwnode)->handle;
> + } else {
> + pr_debug("bad node type for node %pfw\n", fwnode);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + status = acpi_get_handle(scope, refstring, &handle);
> + if (ACPI_FAILURE(status)) {
> + acpi_handle_debug(scope, "can't get handle for %s", refstring);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + device = acpi_fetch_acpi_dev(handle);
> + if (device)
> + return acpi_fwnode_handle(device);
> +
> + status = acpi_get_data_full(handle, acpi_nondev_subnode_tag,
> + (void **)&dn, NULL);
> + if (ACPI_FAILURE(status) || !dn) {
> + acpi_handle_debug(handle, "can't find subnode");
> + return ERR_PTR(-EINVAL);
> + }
> +
> + return &dn->fwnode;
> +}
> +
> /**
> * __acpi_node_get_property_reference - returns handle to the referenced object
> * @fwnode: Firmware node to get the property from
> @@ -886,6 +931,7 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
> const union acpi_object *element, *end;
> const union acpi_object *obj;
> const struct acpi_device_data *data;
> + struct fwnode_handle *ref_fwnode;
> struct acpi_device *device;
> int ret, idx = 0;
>
> @@ -909,16 +955,29 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
>
> args->fwnode = acpi_fwnode_handle(device);
> args->nargs = 0;
> + return 0;
> + case ACPI_TYPE_STRING:
> + if (index)
> + return -ENOENT;
> +
> + ref_fwnode = acpi_parse_string_ref(fwnode, obj->string.pointer);
> + if (IS_ERR(ref_fwnode))
> + return PTR_ERR(ref_fwnode);
> +
> + args->fwnode = ref_fwnode;
> + args->nargs = 0;
> +
> return 0;
> case ACPI_TYPE_PACKAGE:
> /*
> * If it is not a single reference, then it is a package of
> - * references followed by number of ints as follows:
> + * references, followed by number of ints as follows:
> *
> * Package () { REF, INT, REF, INT, INT }
> *
> - * The index argument is then used to determine which reference
> - * the caller wants (along with the arguments).
> + * Here, REF may be either a local reference or a string. The
> + * index argument is then used to determine which reference the
> + * caller wants (along with the arguments).
> */
> break;
> default:
> @@ -942,7 +1001,26 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
>
> ret = acpi_get_ref_args(idx == index ? args : NULL,
> acpi_fwnode_handle(device),
> - &element, end, num_args);
> + &element, end, num_args, true);
> + if (ret < 0)
> + return ret;
> +
> + if (idx == index)
> + return 0;
> +
> + break;
> + case ACPI_TYPE_STRING:
> + ref_fwnode =
> + acpi_parse_string_ref(fwnode,
> + element->string.pointer);
> + if (IS_ERR(ref_fwnode))
> + return PTR_ERR(ref_fwnode);
> +
> + element++;
> +
> + ret = acpi_get_ref_args(idx == index ? args : NULL,
> + ref_fwnode, &element, end,
> + num_args, false);
> if (ret < 0)
> return ret;
>
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v8 02/10] ACPI: property: Parse data node string references in properties
2023-03-29 10:09 ` [PATCH v8 02/10] ACPI: property: Parse data node string references in properties Sakari Ailus
2023-05-11 17:04 ` Rafael J. Wysocki
@ 2023-05-12 16:04 ` Rafael J. Wysocki
2023-05-16 11:24 ` Sakari Ailus
1 sibling, 1 reply; 36+ messages in thread
From: Rafael J. Wysocki @ 2023-05-12 16:04 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-acpi, linux-media, rafael, andriy.shevchenko,
heikki.krogerus
On Wed, Mar 29, 2023 at 12:10 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Add support for parsing property references using strings, besides
> reference objects that were previously supported. This allows also
> referencing data nodes which was not possible with reference objects.
>
> Also add pr_fmt() macro to prefix printouts.
>
> While at it, update copyright.
Although I said that it looked good to me, some minor improvements can
still be made.
First off, the above changelog is a bit terse.
I think that it would help to provide an example of device properties
that would not be parsed properly before the change and can be parsed
now.
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/acpi/property.c | 110 ++++++++++++++++++++++++++++++++++------
> 1 file changed, 94 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index b8d9eb9a433e..08831ffba26c 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -2,14 +2,17 @@
> /*
> * ACPI device specific properties support.
> *
> - * Copyright (C) 2014, Intel Corporation
> + * Copyright (C) 2014-2023, Intel Corporation
> * All rights reserved.
> *
> * Authors: Mika Westerberg <mika.westerberg@linux.intel.com>
> - * Darren Hart <dvhart@linux.intel.com>
> - * Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> + * Darren Hart <dvhart@linux.intel.com>
> + * Rafael J. Wysocki <rafael.j.wysocki@intel.com>
I'm not sure if the whitespace change here is really useful.
> + * Sakari Ailus <sakari.ailus@linux.intel.com>
> */
>
> +#define pr_fmt(fmt) "ACPI: " fmt
> +
> #include <linux/acpi.h>
> #include <linux/device.h>
> #include <linux/export.h>
> @@ -795,7 +798,8 @@ acpi_fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
> static int acpi_get_ref_args(struct fwnode_reference_args *args,
> struct fwnode_handle *ref_fwnode,
> const union acpi_object **element,
> - const union acpi_object *end, size_t num_args)
> + const union acpi_object *end, size_t num_args,
> + bool subnode_string)
The meaning of the new argument isn't really clear. it would be good
to somehow help a casual reader of the code to find this out more
easily.
> {
> u32 nargs = 0, i;
>
> @@ -803,13 +807,16 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args,
> * Find the referred data extension node under the
> * referred device node.
> */
> - for (; *element < end && (*element)->type == ACPI_TYPE_STRING;
> - (*element)++) {
> - const char *child_name = (*element)->string.pointer;
> -
> - ref_fwnode = acpi_fwnode_get_named_child_node(ref_fwnode, child_name);
> - if (!ref_fwnode)
> - return -EINVAL;
> + if (subnode_string) {
> + for (; *element < end && (*element)->type == ACPI_TYPE_STRING;
> + (*element)++) {
> + const char *child_name = (*element)->string.pointer;
> +
> + ref_fwnode = acpi_fwnode_get_named_child_node(ref_fwnode,
> + child_name);
> + if (!ref_fwnode)
> + return -EINVAL;
> + }
> }
>
> /*
> @@ -820,7 +827,8 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args,
> for (i = 0; (*element) + i < end && i < num_args; i++) {
> acpi_object_type type = (*element)[i].type;
>
> - if (type == ACPI_TYPE_LOCAL_REFERENCE)
> + if (type == ACPI_TYPE_LOCAL_REFERENCE ||
> + (!subnode_string && type == ACPI_TYPE_STRING))
> break;
>
> if (type == ACPI_TYPE_INTEGER)
> @@ -844,6 +852,43 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args,
> return 0;
> }
>
> +static struct fwnode_handle *
> +acpi_parse_string_ref(const struct fwnode_handle *fwnode, const char *refstring)
> +{
> + acpi_handle scope, handle;
> + struct acpi_data_node *dn;
> + struct acpi_device *device;
> + acpi_status status;
> +
> + if (is_acpi_device_node(fwnode)) {
> + scope = to_acpi_device_node(fwnode)->handle;
> + } else if (is_acpi_data_node(fwnode)) {
> + scope = to_acpi_data_node(fwnode)->handle;
> + } else {
> + pr_debug("bad node type for node %pfw\n", fwnode);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + status = acpi_get_handle(scope, refstring, &handle);
> + if (ACPI_FAILURE(status)) {
> + acpi_handle_debug(scope, "can't get handle for %s", refstring);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + device = acpi_fetch_acpi_dev(handle);
> + if (device)
> + return acpi_fwnode_handle(device);
> +
> + status = acpi_get_data_full(handle, acpi_nondev_subnode_tag,
> + (void **)&dn, NULL);
> + if (ACPI_FAILURE(status) || !dn) {
> + acpi_handle_debug(handle, "can't find subnode");
> + return ERR_PTR(-EINVAL);
> + }
> +
> + return &dn->fwnode;
So on failure this function always returns the same error code. Can
it return NULL instead which can be translated into an error code by
the caller?
> +}
> +
> /**
> * __acpi_node_get_property_reference - returns handle to the referenced object
> * @fwnode: Firmware node to get the property from
> @@ -886,6 +931,7 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
> const union acpi_object *element, *end;
> const union acpi_object *obj;
> const struct acpi_device_data *data;
> + struct fwnode_handle *ref_fwnode;
> struct acpi_device *device;
> int ret, idx = 0;
>
> @@ -909,16 +955,29 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
>
> args->fwnode = acpi_fwnode_handle(device);
> args->nargs = 0;
> + return 0;
> + case ACPI_TYPE_STRING:
> + if (index)
> + return -ENOENT;
> +
> + ref_fwnode = acpi_parse_string_ref(fwnode, obj->string.pointer);
> + if (IS_ERR(ref_fwnode))
> + return PTR_ERR(ref_fwnode);
> +
> + args->fwnode = ref_fwnode;
> + args->nargs = 0;
> +
> return 0;
> case ACPI_TYPE_PACKAGE:
> /*
> * If it is not a single reference, then it is a package of
> - * references followed by number of ints as follows:
> + * references, followed by number of ints as follows:
> *
> * Package () { REF, INT, REF, INT, INT }
> *
> - * The index argument is then used to determine which reference
> - * the caller wants (along with the arguments).
> + * Here, REF may be either a local reference or a string. The
> + * index argument is then used to determine which reference the
> + * caller wants (along with the arguments).
> */
> break;
> default:
> @@ -942,7 +1001,26 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
>
> ret = acpi_get_ref_args(idx == index ? args : NULL,
> acpi_fwnode_handle(device),
> - &element, end, num_args);
> + &element, end, num_args, true);
> + if (ret < 0)
> + return ret;
> +
> + if (idx == index)
> + return 0;
> +
> + break;
> + case ACPI_TYPE_STRING:
> + ref_fwnode =
> + acpi_parse_string_ref(fwnode,
> + element->string.pointer);
> + if (IS_ERR(ref_fwnode))
> + return PTR_ERR(ref_fwnode);
> +
> + element++;
> +
> + ret = acpi_get_ref_args(idx == index ? args : NULL,
> + ref_fwnode, &element, end,
> + num_args, false);
> if (ret < 0)
> return ret;
>
> --
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v8 02/10] ACPI: property: Parse data node string references in properties
2023-05-12 16:04 ` Rafael J. Wysocki
@ 2023-05-16 11:24 ` Sakari Ailus
2023-05-22 15:29 ` Rafael J. Wysocki
0 siblings, 1 reply; 36+ messages in thread
From: Sakari Ailus @ 2023-05-16 11:24 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: linux-acpi, linux-media, andriy.shevchenko, heikki.krogerus
Hi Rafael,
On Fri, May 12, 2023 at 06:04:26PM +0200, Rafael J. Wysocki wrote:
> On Wed, Mar 29, 2023 at 12:10 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Add support for parsing property references using strings, besides
> > reference objects that were previously supported. This allows also
> > referencing data nodes which was not possible with reference objects.
> >
> > Also add pr_fmt() macro to prefix printouts.
> >
> > While at it, update copyright.
>
> Although I said that it looked good to me, some minor improvements can
> still be made.
>
> First off, the above changelog is a bit terse.
>
> I think that it would help to provide an example of device properties
> that would not be parsed properly before the change and can be parsed
> now.
>
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > drivers/acpi/property.c | 110 ++++++++++++++++++++++++++++++++++------
> > 1 file changed, 94 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > index b8d9eb9a433e..08831ffba26c 100644
> > --- a/drivers/acpi/property.c
> > +++ b/drivers/acpi/property.c
> > @@ -2,14 +2,17 @@
> > /*
> > * ACPI device specific properties support.
> > *
> > - * Copyright (C) 2014, Intel Corporation
> > + * Copyright (C) 2014-2023, Intel Corporation
> > * All rights reserved.
> > *
> > * Authors: Mika Westerberg <mika.westerberg@linux.intel.com>
> > - * Darren Hart <dvhart@linux.intel.com>
> > - * Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > + * Darren Hart <dvhart@linux.intel.com>
> > + * Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> I'm not sure if the whitespace change here is really useful.
I did that to address a comment from Andy --- the earlier lines used spaces
for indentation.
>
> > + * Sakari Ailus <sakari.ailus@linux.intel.com>
> > */
> >
> > +#define pr_fmt(fmt) "ACPI: " fmt
> > +
> > #include <linux/acpi.h>
> > #include <linux/device.h>
> > #include <linux/export.h>
> > @@ -795,7 +798,8 @@ acpi_fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
> > static int acpi_get_ref_args(struct fwnode_reference_args *args,
> > struct fwnode_handle *ref_fwnode,
> > const union acpi_object **element,
> > - const union acpi_object *end, size_t num_args)
> > + const union acpi_object *end, size_t num_args,
> > + bool subnode_string)
>
> The meaning of the new argument isn't really clear. it would be good
> to somehow help a casual reader of the code to find this out more
> easily.
I can add comments to v9.
>
> > {
> > u32 nargs = 0, i;
> >
> > @@ -803,13 +807,16 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args,
> > * Find the referred data extension node under the
> > * referred device node.
> > */
> > - for (; *element < end && (*element)->type == ACPI_TYPE_STRING;
> > - (*element)++) {
> > - const char *child_name = (*element)->string.pointer;
> > -
> > - ref_fwnode = acpi_fwnode_get_named_child_node(ref_fwnode, child_name);
> > - if (!ref_fwnode)
> > - return -EINVAL;
> > + if (subnode_string) {
> > + for (; *element < end && (*element)->type == ACPI_TYPE_STRING;
> > + (*element)++) {
> > + const char *child_name = (*element)->string.pointer;
> > +
> > + ref_fwnode = acpi_fwnode_get_named_child_node(ref_fwnode,
> > + child_name);
> > + if (!ref_fwnode)
> > + return -EINVAL;
> > + }
> > }
> >
> > /*
> > @@ -820,7 +827,8 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args,
> > for (i = 0; (*element) + i < end && i < num_args; i++) {
> > acpi_object_type type = (*element)[i].type;
> >
> > - if (type == ACPI_TYPE_LOCAL_REFERENCE)
> > + if (type == ACPI_TYPE_LOCAL_REFERENCE ||
> > + (!subnode_string && type == ACPI_TYPE_STRING))
> > break;
> >
> > if (type == ACPI_TYPE_INTEGER)
> > @@ -844,6 +852,43 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args,
> > return 0;
> > }
> >
> > +static struct fwnode_handle *
> > +acpi_parse_string_ref(const struct fwnode_handle *fwnode, const char *refstring)
> > +{
> > + acpi_handle scope, handle;
> > + struct acpi_data_node *dn;
> > + struct acpi_device *device;
> > + acpi_status status;
> > +
> > + if (is_acpi_device_node(fwnode)) {
> > + scope = to_acpi_device_node(fwnode)->handle;
> > + } else if (is_acpi_data_node(fwnode)) {
> > + scope = to_acpi_data_node(fwnode)->handle;
> > + } else {
> > + pr_debug("bad node type for node %pfw\n", fwnode);
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > + status = acpi_get_handle(scope, refstring, &handle);
> > + if (ACPI_FAILURE(status)) {
> > + acpi_handle_debug(scope, "can't get handle for %s", refstring);
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > + device = acpi_fetch_acpi_dev(handle);
> > + if (device)
> > + return acpi_fwnode_handle(device);
> > +
> > + status = acpi_get_data_full(handle, acpi_nondev_subnode_tag,
> > + (void **)&dn, NULL);
> > + if (ACPI_FAILURE(status) || !dn) {
> > + acpi_handle_debug(handle, "can't find subnode");
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > + return &dn->fwnode;
>
> So on failure this function always returns the same error code. Can
> it return NULL instead which can be translated into an error code by
> the caller?
Sure, makes sense.
>
> > +}
> > +
> > /**
> > * __acpi_node_get_property_reference - returns handle to the referenced object
> > * @fwnode: Firmware node to get the property from
> > @@ -886,6 +931,7 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
> > const union acpi_object *element, *end;
> > const union acpi_object *obj;
> > const struct acpi_device_data *data;
> > + struct fwnode_handle *ref_fwnode;
> > struct acpi_device *device;
> > int ret, idx = 0;
> >
> > @@ -909,16 +955,29 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
> >
> > args->fwnode = acpi_fwnode_handle(device);
> > args->nargs = 0;
> > + return 0;
> > + case ACPI_TYPE_STRING:
> > + if (index)
> > + return -ENOENT;
> > +
> > + ref_fwnode = acpi_parse_string_ref(fwnode, obj->string.pointer);
> > + if (IS_ERR(ref_fwnode))
> > + return PTR_ERR(ref_fwnode);
> > +
> > + args->fwnode = ref_fwnode;
> > + args->nargs = 0;
> > +
> > return 0;
> > case ACPI_TYPE_PACKAGE:
> > /*
> > * If it is not a single reference, then it is a package of
> > - * references followed by number of ints as follows:
> > + * references, followed by number of ints as follows:
> > *
> > * Package () { REF, INT, REF, INT, INT }
> > *
> > - * The index argument is then used to determine which reference
> > - * the caller wants (along with the arguments).
> > + * Here, REF may be either a local reference or a string. The
> > + * index argument is then used to determine which reference the
> > + * caller wants (along with the arguments).
> > */
> > break;
> > default:
> > @@ -942,7 +1001,26 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
> >
> > ret = acpi_get_ref_args(idx == index ? args : NULL,
> > acpi_fwnode_handle(device),
> > - &element, end, num_args);
> > + &element, end, num_args, true);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (idx == index)
> > + return 0;
> > +
> > + break;
> > + case ACPI_TYPE_STRING:
> > + ref_fwnode =
> > + acpi_parse_string_ref(fwnode,
> > + element->string.pointer);
> > + if (IS_ERR(ref_fwnode))
> > + return PTR_ERR(ref_fwnode);
> > +
> > + element++;
> > +
> > + ret = acpi_get_ref_args(idx == index ? args : NULL,
> > + ref_fwnode, &element, end,
> > + num_args, false);
> > if (ret < 0)
> > return ret;
> >
> > --
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v8 02/10] ACPI: property: Parse data node string references in properties
2023-05-16 11:24 ` Sakari Ailus
@ 2023-05-22 15:29 ` Rafael J. Wysocki
2023-05-22 16:28 ` Sakari Ailus
0 siblings, 1 reply; 36+ messages in thread
From: Rafael J. Wysocki @ 2023-05-22 15:29 UTC (permalink / raw)
To: Sakari Ailus
Cc: Rafael J. Wysocki, linux-acpi, linux-media, andriy.shevchenko,
heikki.krogerus
On Tue, May 16, 2023 at 1:24 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Rafael,
>
> On Fri, May 12, 2023 at 06:04:26PM +0200, Rafael J. Wysocki wrote:
> > On Wed, Mar 29, 2023 at 12:10 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Add support for parsing property references using strings, besides
> > > reference objects that were previously supported. This allows also
> > > referencing data nodes which was not possible with reference objects.
> > >
> > > Also add pr_fmt() macro to prefix printouts.
> > >
> > > While at it, update copyright.
> >
> > Although I said that it looked good to me, some minor improvements can
> > still be made.
> >
> > First off, the above changelog is a bit terse.
> >
> > I think that it would help to provide an example of device properties
> > that would not be parsed properly before the change and can be parsed
> > now.
> >
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > > drivers/acpi/property.c | 110 ++++++++++++++++++++++++++++++++++------
> > > 1 file changed, 94 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > > index b8d9eb9a433e..08831ffba26c 100644
> > > --- a/drivers/acpi/property.c
> > > +++ b/drivers/acpi/property.c
> > > @@ -2,14 +2,17 @@
> > > /*
> > > * ACPI device specific properties support.
> > > *
> > > - * Copyright (C) 2014, Intel Corporation
> > > + * Copyright (C) 2014-2023, Intel Corporation
> > > * All rights reserved.
> > > *
> > > * Authors: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > - * Darren Hart <dvhart@linux.intel.com>
> > > - * Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > + * Darren Hart <dvhart@linux.intel.com>
> > > + * Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > I'm not sure if the whitespace change here is really useful.
>
> I did that to address a comment from Andy --- the earlier lines used spaces
> for indentation.
>
> >
> > > + * Sakari Ailus <sakari.ailus@linux.intel.com>
> > > */
> > >
> > > +#define pr_fmt(fmt) "ACPI: " fmt
> > > +
> > > #include <linux/acpi.h>
> > > #include <linux/device.h>
> > > #include <linux/export.h>
> > > @@ -795,7 +798,8 @@ acpi_fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
> > > static int acpi_get_ref_args(struct fwnode_reference_args *args,
> > > struct fwnode_handle *ref_fwnode,
> > > const union acpi_object **element,
> > > - const union acpi_object *end, size_t num_args)
> > > + const union acpi_object *end, size_t num_args,
> > > + bool subnode_string)
> >
> > The meaning of the new argument isn't really clear. it would be good
> > to somehow help a casual reader of the code to find this out more
> > easily.
>
> I can add comments to v9.
If you can send me an example of ASL that will be parsed correctly
after this change, but not before, it will help a bit.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v8 02/10] ACPI: property: Parse data node string references in properties
2023-05-22 15:29 ` Rafael J. Wysocki
@ 2023-05-22 16:28 ` Sakari Ailus
2023-05-22 16:38 ` Rafael J. Wysocki
0 siblings, 1 reply; 36+ messages in thread
From: Sakari Ailus @ 2023-05-22 16:28 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: linux-acpi, linux-media, andriy.shevchenko, heikki.krogerus
Hi Rafael,
On Mon, May 22, 2023 at 05:29:48PM +0200, Rafael J. Wysocki wrote:
> On Tue, May 16, 2023 at 1:24 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Rafael,
> >
> > On Fri, May 12, 2023 at 06:04:26PM +0200, Rafael J. Wysocki wrote:
> > > On Wed, Mar 29, 2023 at 12:10 PM Sakari Ailus
> > > <sakari.ailus@linux.intel.com> wrote:
> > > >
> > > > Add support for parsing property references using strings, besides
> > > > reference objects that were previously supported. This allows also
> > > > referencing data nodes which was not possible with reference objects.
> > > >
> > > > Also add pr_fmt() macro to prefix printouts.
> > > >
> > > > While at it, update copyright.
> > >
> > > Although I said that it looked good to me, some minor improvements can
> > > still be made.
> > >
> > > First off, the above changelog is a bit terse.
> > >
> > > I think that it would help to provide an example of device properties
> > > that would not be parsed properly before the change and can be parsed
> > > now.
> > >
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > ---
> > > > drivers/acpi/property.c | 110 ++++++++++++++++++++++++++++++++++------
> > > > 1 file changed, 94 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > > > index b8d9eb9a433e..08831ffba26c 100644
> > > > --- a/drivers/acpi/property.c
> > > > +++ b/drivers/acpi/property.c
> > > > @@ -2,14 +2,17 @@
> > > > /*
> > > > * ACPI device specific properties support.
> > > > *
> > > > - * Copyright (C) 2014, Intel Corporation
> > > > + * Copyright (C) 2014-2023, Intel Corporation
> > > > * All rights reserved.
> > > > *
> > > > * Authors: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > - * Darren Hart <dvhart@linux.intel.com>
> > > > - * Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > + * Darren Hart <dvhart@linux.intel.com>
> > > > + * Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > I'm not sure if the whitespace change here is really useful.
> >
> > I did that to address a comment from Andy --- the earlier lines used spaces
> > for indentation.
> >
> > >
> > > > + * Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > */
> > > >
> > > > +#define pr_fmt(fmt) "ACPI: " fmt
> > > > +
> > > > #include <linux/acpi.h>
> > > > #include <linux/device.h>
> > > > #include <linux/export.h>
> > > > @@ -795,7 +798,8 @@ acpi_fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
> > > > static int acpi_get_ref_args(struct fwnode_reference_args *args,
> > > > struct fwnode_handle *ref_fwnode,
> > > > const union acpi_object **element,
> > > > - const union acpi_object *end, size_t num_args)
> > > > + const union acpi_object *end, size_t num_args,
> > > > + bool subnode_string)
> > >
> > > The meaning of the new argument isn't really clear. it would be good
> > > to somehow help a casual reader of the code to find this out more
> > > easily.
> >
> > I can add comments to v9.
>
> If you can send me an example of ASL that will be parsed correctly
> after this change, but not before, it will help a bit.
E.g. this bit from DisCo for Imaging 1.0 (Annex B.1):
Package () {
"mipi-img-flash-leds",
Package () {
"\\_SB.PCI0.I2C2.LEDD.LED0",
"\\_SB.PCI0.I2C2.LEDD.LED1"
},
},
It's a property with a string reference to an ACPI non-device node,
although you can refer to device nodes as well.
You can get the spec from here:
<URL:https://www.mipi.org/mipi-disco-for-imaging-download>.
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v8 02/10] ACPI: property: Parse data node string references in properties
2023-05-22 16:28 ` Sakari Ailus
@ 2023-05-22 16:38 ` Rafael J. Wysocki
2023-05-22 20:25 ` Sakari Ailus
0 siblings, 1 reply; 36+ messages in thread
From: Rafael J. Wysocki @ 2023-05-22 16:38 UTC (permalink / raw)
To: Sakari Ailus
Cc: Rafael J. Wysocki, linux-acpi, linux-media, andriy.shevchenko,
heikki.krogerus
Hi Sakari,
On Mon, May 22, 2023 at 6:28 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
> On Mon, May 22, 2023 at 05:29:48PM +0200, Rafael J. Wysocki wrote:
> > On Tue, May 16, 2023 at 1:24 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > > On Fri, May 12, 2023 at 06:04:26PM +0200, Rafael J. Wysocki wrote:
> > > > On Wed, Mar 29, 2023 at 12:10 PM Sakari Ailus
> > > > <sakari.ailus@linux.intel.com> wrote:
> > > > >
> > > > > Add support for parsing property references using strings, besides
> > > > > reference objects that were previously supported. This allows also
> > > > > referencing data nodes which was not possible with reference objects.
> > > > >
> > > > > Also add pr_fmt() macro to prefix printouts.
> > > > >
> > > > > While at it, update copyright.
> > > >
> > > > Although I said that it looked good to me, some minor improvements can
> > > > still be made.
> > > >
> > > > First off, the above changelog is a bit terse.
> > > >
> > > > I think that it would help to provide an example of device properties
> > > > that would not be parsed properly before the change and can be parsed
> > > > now.
> > > >
> > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > ---
> > > > > drivers/acpi/property.c | 110 ++++++++++++++++++++++++++++++++++------
> > > > > 1 file changed, 94 insertions(+), 16 deletions(-)
> > > > >
> > > > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > > > > index b8d9eb9a433e..08831ffba26c 100644
> > > > > --- a/drivers/acpi/property.c
> > > > > +++ b/drivers/acpi/property.c
> > > > > @@ -2,14 +2,17 @@
> > > > > /*
> > > > > * ACPI device specific properties support.
> > > > > *
> > > > > - * Copyright (C) 2014, Intel Corporation
> > > > > + * Copyright (C) 2014-2023, Intel Corporation
> > > > > * All rights reserved.
> > > > > *
> > > > > * Authors: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > > - * Darren Hart <dvhart@linux.intel.com>
> > > > > - * Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > + * Darren Hart <dvhart@linux.intel.com>
> > > > > + * Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > >
> > > > I'm not sure if the whitespace change here is really useful.
> > >
> > > I did that to address a comment from Andy --- the earlier lines used spaces
> > > for indentation.
> > >
> > > >
> > > > > + * Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > */
> > > > >
> > > > > +#define pr_fmt(fmt) "ACPI: " fmt
> > > > > +
> > > > > #include <linux/acpi.h>
> > > > > #include <linux/device.h>
> > > > > #include <linux/export.h>
> > > > > @@ -795,7 +798,8 @@ acpi_fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
> > > > > static int acpi_get_ref_args(struct fwnode_reference_args *args,
> > > > > struct fwnode_handle *ref_fwnode,
> > > > > const union acpi_object **element,
> > > > > - const union acpi_object *end, size_t num_args)
> > > > > + const union acpi_object *end, size_t num_args,
> > > > > + bool subnode_string)
> > > >
> > > > The meaning of the new argument isn't really clear. it would be good
> > > > to somehow help a casual reader of the code to find this out more
> > > > easily.
> > >
> > > I can add comments to v9.
> >
> > If you can send me an example of ASL that will be parsed correctly
> > after this change, but not before, it will help a bit.
>
> E.g. this bit from DisCo for Imaging 1.0 (Annex B.1):
>
> Package () {
> "mipi-img-flash-leds",
> Package () {
> "\\_SB.PCI0.I2C2.LEDD.LED0",
> "\\_SB.PCI0.I2C2.LEDD.LED1"
> },
> },
>
> It's a property with a string reference to an ACPI non-device node,
> although you can refer to device nodes as well.
This example is missing the definition of LED0 or LED1 from which it
would be clear that they are data nodes (or at least one of them is a
data node).
Also I'm kind of wondering about the "reference with arguments" part
which seems to work differently depending on whether the reference is
represented by a string or by a reference object.
> You can get the spec from here:
> <URL:https://www.mipi.org/mipi-disco-for-imaging-download>.
Sure, but it alone won't help me much with documenting this code change.
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v8 02/10] ACPI: property: Parse data node string references in properties
2023-05-22 16:38 ` Rafael J. Wysocki
@ 2023-05-22 20:25 ` Sakari Ailus
2023-05-23 11:21 ` Rafael J. Wysocki
0 siblings, 1 reply; 36+ messages in thread
From: Sakari Ailus @ 2023-05-22 20:25 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: linux-acpi, linux-media, andriy.shevchenko, heikki.krogerus
Hi Rafael,
On Mon, May 22, 2023 at 06:38:37PM +0200, Rafael J. Wysocki wrote:
> Hi Sakari,
>
> On Mon, May 22, 2023 at 6:28 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> > On Mon, May 22, 2023 at 05:29:48PM +0200, Rafael J. Wysocki wrote:
> > > On Tue, May 16, 2023 at 1:24 PM Sakari Ailus
> > > <sakari.ailus@linux.intel.com> wrote:
> > > > On Fri, May 12, 2023 at 06:04:26PM +0200, Rafael J. Wysocki wrote:
> > > > > On Wed, Mar 29, 2023 at 12:10 PM Sakari Ailus
> > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > >
> > > > > > Add support for parsing property references using strings, besides
> > > > > > reference objects that were previously supported. This allows also
> > > > > > referencing data nodes which was not possible with reference objects.
> > > > > >
> > > > > > Also add pr_fmt() macro to prefix printouts.
> > > > > >
> > > > > > While at it, update copyright.
> > > > >
> > > > > Although I said that it looked good to me, some minor improvements can
> > > > > still be made.
> > > > >
> > > > > First off, the above changelog is a bit terse.
> > > > >
> > > > > I think that it would help to provide an example of device properties
> > > > > that would not be parsed properly before the change and can be parsed
> > > > > now.
> > > > >
> > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > ---
> > > > > > drivers/acpi/property.c | 110 ++++++++++++++++++++++++++++++++++------
> > > > > > 1 file changed, 94 insertions(+), 16 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > > > > > index b8d9eb9a433e..08831ffba26c 100644
> > > > > > --- a/drivers/acpi/property.c
> > > > > > +++ b/drivers/acpi/property.c
> > > > > > @@ -2,14 +2,17 @@
> > > > > > /*
> > > > > > * ACPI device specific properties support.
> > > > > > *
> > > > > > - * Copyright (C) 2014, Intel Corporation
> > > > > > + * Copyright (C) 2014-2023, Intel Corporation
> > > > > > * All rights reserved.
> > > > > > *
> > > > > > * Authors: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > > > - * Darren Hart <dvhart@linux.intel.com>
> > > > > > - * Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > + * Darren Hart <dvhart@linux.intel.com>
> > > > > > + * Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > >
> > > > > I'm not sure if the whitespace change here is really useful.
> > > >
> > > > I did that to address a comment from Andy --- the earlier lines used spaces
> > > > for indentation.
> > > >
> > > > >
> > > > > > + * Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > */
> > > > > >
> > > > > > +#define pr_fmt(fmt) "ACPI: " fmt
> > > > > > +
> > > > > > #include <linux/acpi.h>
> > > > > > #include <linux/device.h>
> > > > > > #include <linux/export.h>
> > > > > > @@ -795,7 +798,8 @@ acpi_fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
> > > > > > static int acpi_get_ref_args(struct fwnode_reference_args *args,
> > > > > > struct fwnode_handle *ref_fwnode,
> > > > > > const union acpi_object **element,
> > > > > > - const union acpi_object *end, size_t num_args)
> > > > > > + const union acpi_object *end, size_t num_args,
> > > > > > + bool subnode_string)
> > > > >
> > > > > The meaning of the new argument isn't really clear. it would be good
> > > > > to somehow help a casual reader of the code to find this out more
> > > > > easily.
> > > >
> > > > I can add comments to v9.
> > >
> > > If you can send me an example of ASL that will be parsed correctly
> > > after this change, but not before, it will help a bit.
> >
> > E.g. this bit from DisCo for Imaging 1.0 (Annex B.1):
> >
> > Package () {
> > "mipi-img-flash-leds",
> > Package () {
> > "\\_SB.PCI0.I2C2.LEDD.LED0",
> > "\\_SB.PCI0.I2C2.LEDD.LED1"
> > },
> > },
> >
> > It's a property with a string reference to an ACPI non-device node,
> > although you can refer to device nodes as well.
>
> This example is missing the definition of LED0 or LED1 from which it
> would be clear that they are data nodes (or at least one of them is a
> data node).
Ok, perhaps this one could work as a complete example, with a single
reference:
Package ()
{
"mipi-img-flash-leds", "\\_SB.PCI0.I2C2.LEDD.LED0",
}
Device (LEDD)
{
Name (_DSD, Package () // _DSD: Device-Specific Data
{
ToUUID ("dbb8e3e6-5886-4ba6-8795-1319f52a966b"), /* Hierarchical Data Extension */,
Package ()
{
Package ()
{
"mipi-img-flash-led-0",
"LED0",
}
},
})
Name (LED0, Package () // _DSD: Device-Specific Data
{
ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */,
Package ()
{
Package ()
{
"mipi-img-max-current",
1000000,
}
}
})
}
>
> Also I'm kind of wondering about the "reference with arguments" part
> which seems to work differently depending on whether the reference is
> represented by a string or by a reference object.
Yes. With (device) reference objects, it is possible currently to refer to
subnodes with the _DSD data extension child names of those nodes. This is
not done with string references as 1) any node can already be referenced so
there's no need to and 2) as node references are strings already, it's not
possible to distinguish node string references from _DSD data node names.
E.g.
"\\_SB.I2C0.LED0", "LED1"
^ ACPI object name or _DSD data node name?
>
> > You can get the spec from here:
> > <URL:https://www.mipi.org/mipi-disco-for-imaging-download>.
>
> Sure, but it alone won't help me much with documenting this code change.
Not as such but the spec is publicly available now.
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v8 02/10] ACPI: property: Parse data node string references in properties
2023-05-22 20:25 ` Sakari Ailus
@ 2023-05-23 11:21 ` Rafael J. Wysocki
2023-05-23 11:43 ` Sakari Ailus
0 siblings, 1 reply; 36+ messages in thread
From: Rafael J. Wysocki @ 2023-05-23 11:21 UTC (permalink / raw)
To: Sakari Ailus
Cc: Rafael J. Wysocki, linux-acpi, linux-media, andriy.shevchenko,
heikki.krogerus
Hi Sakari,
On Mon, May 22, 2023 at 10:35 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Rafael,
>
> On Mon, May 22, 2023 at 06:38:37PM +0200, Rafael J. Wysocki wrote:
> > On Mon, May 22, 2023 at 6:28 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > > On Mon, May 22, 2023 at 05:29:48PM +0200, Rafael J. Wysocki wrote:
> > > > On Tue, May 16, 2023 at 1:24 PM Sakari Ailus
> > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > On Fri, May 12, 2023 at 06:04:26PM +0200, Rafael J. Wysocki wrote:
> > > > > > On Wed, Mar 29, 2023 at 12:10 PM Sakari Ailus
> > > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > > >
> > > > > > > Add support for parsing property references using strings, besides
> > > > > > > reference objects that were previously supported. This allows also
> > > > > > > referencing data nodes which was not possible with reference objects.
> > > > > > >
> > > > > > > Also add pr_fmt() macro to prefix printouts.
> > > > > > >
> > > > > > > While at it, update copyright.
> > > > > >
> > > > > > Although I said that it looked good to me, some minor improvements can
> > > > > > still be made.
> > > > > >
> > > > > > First off, the above changelog is a bit terse.
> > > > > >
> > > > > > I think that it would help to provide an example of device properties
> > > > > > that would not be parsed properly before the change and can be parsed
> > > > > > now.
> > > > > >
> > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > > ---
> > > > > > > drivers/acpi/property.c | 110 ++++++++++++++++++++++++++++++++++------
> > > > > > > 1 file changed, 94 insertions(+), 16 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > > > > > > index b8d9eb9a433e..08831ffba26c 100644
> > > > > > > --- a/drivers/acpi/property.c
> > > > > > > +++ b/drivers/acpi/property.c
> > > > > > > @@ -2,14 +2,17 @@
> > > > > > > /*
> > > > > > > * ACPI device specific properties support.
> > > > > > > *
> > > > > > > - * Copyright (C) 2014, Intel Corporation
> > > > > > > + * Copyright (C) 2014-2023, Intel Corporation
> > > > > > > * All rights reserved.
> > > > > > > *
> > > > > > > * Authors: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > > > > - * Darren Hart <dvhart@linux.intel.com>
> > > > > > > - * Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > > + * Darren Hart <dvhart@linux.intel.com>
> > > > > > > + * Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > >
> > > > > > I'm not sure if the whitespace change here is really useful.
> > > > >
> > > > > I did that to address a comment from Andy --- the earlier lines used spaces
> > > > > for indentation.
> > > > >
> > > > > >
> > > > > > > + * Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > > */
> > > > > > >
> > > > > > > +#define pr_fmt(fmt) "ACPI: " fmt
> > > > > > > +
> > > > > > > #include <linux/acpi.h>
> > > > > > > #include <linux/device.h>
> > > > > > > #include <linux/export.h>
> > > > > > > @@ -795,7 +798,8 @@ acpi_fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
> > > > > > > static int acpi_get_ref_args(struct fwnode_reference_args *args,
> > > > > > > struct fwnode_handle *ref_fwnode,
> > > > > > > const union acpi_object **element,
> > > > > > > - const union acpi_object *end, size_t num_args)
> > > > > > > + const union acpi_object *end, size_t num_args,
> > > > > > > + bool subnode_string)
> > > > > >
> > > > > > The meaning of the new argument isn't really clear. it would be good
> > > > > > to somehow help a casual reader of the code to find this out more
> > > > > > easily.
> > > > >
> > > > > I can add comments to v9.
> > > >
> > > > If you can send me an example of ASL that will be parsed correctly
> > > > after this change, but not before, it will help a bit.
> > >
> > > E.g. this bit from DisCo for Imaging 1.0 (Annex B.1):
> > >
> > > Package () {
> > > "mipi-img-flash-leds",
> > > Package () {
> > > "\\_SB.PCI0.I2C2.LEDD.LED0",
> > > "\\_SB.PCI0.I2C2.LEDD.LED1"
> > > },
> > > },
> > >
> > > It's a property with a string reference to an ACPI non-device node,
> > > although you can refer to device nodes as well.
> >
> > This example is missing the definition of LED0 or LED1 from which it
> > would be clear that they are data nodes (or at least one of them is a
> > data node).
>
> Ok, perhaps this one could work as a complete example, with a single
> reference:
>
> Package ()
> {
> "mipi-img-flash-leds", "\\_SB.PCI0.I2C2.LEDD.LED0",
> }
>
> Device (LEDD)
> {
> Name (_DSD, Package () // _DSD: Device-Specific Data
> {
> ToUUID ("dbb8e3e6-5886-4ba6-8795-1319f52a966b"), /* Hierarchical Data Extension */,
> Package ()
> {
> Package ()
> {
> "mipi-img-flash-led-0",
> "LED0",
> }
> },
> })
> Name (LED0, Package () // _DSD: Device-Specific Data
> {
> ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */,
> Package ()
> {
> Package ()
> {
> "mipi-img-max-current",
> 1000000,
> }
> }
> })
> }
>
This works, thanks!
> >
> > Also I'm kind of wondering about the "reference with arguments" part
> > which seems to work differently depending on whether the reference is
> > represented by a string or by a reference object.
>
> Yes. With (device) reference objects, it is possible currently to refer to
> subnodes with the _DSD data extension child names of those nodes. This is
> not done with string references as 1) any node can already be referenced so
> there's no need to and 2) as node references are strings already, it's not
> possible to distinguish node string references from _DSD data node names.
> E.g.
>
> "\\_SB.I2C0.LED0", "LED1"
>
> ^ ACPI object name or _DSD data node name?
>
Has this behavior been documented anywhere? Or is there any
expectation to see anything like this shipping in production platform
firmware?
If any of the above isn't the case, I would be inclined to simply
remove this special case and make both the "object reference" and
"string" cases work in the same way and if someone needs to refer to a
data node, they will just need to use a string (in which case it will
be the only option).
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v8 02/10] ACPI: property: Parse data node string references in properties
2023-05-23 11:21 ` Rafael J. Wysocki
@ 2023-05-23 11:43 ` Sakari Ailus
2023-05-23 13:40 ` Rafael J. Wysocki
0 siblings, 1 reply; 36+ messages in thread
From: Sakari Ailus @ 2023-05-23 11:43 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: linux-acpi, linux-media, andriy.shevchenko, heikki.krogerus
Hi Rafael,
On Tue, May 23, 2023 at 01:21:12PM +0200, Rafael J. Wysocki wrote:
> Hi Sakari,
>
> On Mon, May 22, 2023 at 10:35 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Rafael,
> >
> > On Mon, May 22, 2023 at 06:38:37PM +0200, Rafael J. Wysocki wrote:
> > > On Mon, May 22, 2023 at 6:28 PM Sakari Ailus
> > > <sakari.ailus@linux.intel.com> wrote:
> > > > On Mon, May 22, 2023 at 05:29:48PM +0200, Rafael J. Wysocki wrote:
> > > > > On Tue, May 16, 2023 at 1:24 PM Sakari Ailus
> > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > > On Fri, May 12, 2023 at 06:04:26PM +0200, Rafael J. Wysocki wrote:
> > > > > > > On Wed, Mar 29, 2023 at 12:10 PM Sakari Ailus
> > > > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > > > >
> > > > > > > > Add support for parsing property references using strings, besides
> > > > > > > > reference objects that were previously supported. This allows also
> > > > > > > > referencing data nodes which was not possible with reference objects.
> > > > > > > >
> > > > > > > > Also add pr_fmt() macro to prefix printouts.
> > > > > > > >
> > > > > > > > While at it, update copyright.
> > > > > > >
> > > > > > > Although I said that it looked good to me, some minor improvements can
> > > > > > > still be made.
> > > > > > >
> > > > > > > First off, the above changelog is a bit terse.
> > > > > > >
> > > > > > > I think that it would help to provide an example of device properties
> > > > > > > that would not be parsed properly before the change and can be parsed
> > > > > > > now.
> > > > > > >
> > > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > > > ---
> > > > > > > > drivers/acpi/property.c | 110 ++++++++++++++++++++++++++++++++++------
> > > > > > > > 1 file changed, 94 insertions(+), 16 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > > > > > > > index b8d9eb9a433e..08831ffba26c 100644
> > > > > > > > --- a/drivers/acpi/property.c
> > > > > > > > +++ b/drivers/acpi/property.c
> > > > > > > > @@ -2,14 +2,17 @@
> > > > > > > > /*
> > > > > > > > * ACPI device specific properties support.
> > > > > > > > *
> > > > > > > > - * Copyright (C) 2014, Intel Corporation
> > > > > > > > + * Copyright (C) 2014-2023, Intel Corporation
> > > > > > > > * All rights reserved.
> > > > > > > > *
> > > > > > > > * Authors: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > > > > > - * Darren Hart <dvhart@linux.intel.com>
> > > > > > > > - * Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > > > + * Darren Hart <dvhart@linux.intel.com>
> > > > > > > > + * Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > >
> > > > > > > I'm not sure if the whitespace change here is really useful.
> > > > > >
> > > > > > I did that to address a comment from Andy --- the earlier lines used spaces
> > > > > > for indentation.
> > > > > >
> > > > > > >
> > > > > > > > + * Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > > > */
> > > > > > > >
> > > > > > > > +#define pr_fmt(fmt) "ACPI: " fmt
> > > > > > > > +
> > > > > > > > #include <linux/acpi.h>
> > > > > > > > #include <linux/device.h>
> > > > > > > > #include <linux/export.h>
> > > > > > > > @@ -795,7 +798,8 @@ acpi_fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
> > > > > > > > static int acpi_get_ref_args(struct fwnode_reference_args *args,
> > > > > > > > struct fwnode_handle *ref_fwnode,
> > > > > > > > const union acpi_object **element,
> > > > > > > > - const union acpi_object *end, size_t num_args)
> > > > > > > > + const union acpi_object *end, size_t num_args,
> > > > > > > > + bool subnode_string)
> > > > > > >
> > > > > > > The meaning of the new argument isn't really clear. it would be good
> > > > > > > to somehow help a casual reader of the code to find this out more
> > > > > > > easily.
> > > > > >
> > > > > > I can add comments to v9.
> > > > >
> > > > > If you can send me an example of ASL that will be parsed correctly
> > > > > after this change, but not before, it will help a bit.
> > > >
> > > > E.g. this bit from DisCo for Imaging 1.0 (Annex B.1):
> > > >
> > > > Package () {
> > > > "mipi-img-flash-leds",
> > > > Package () {
> > > > "\\_SB.PCI0.I2C2.LEDD.LED0",
> > > > "\\_SB.PCI0.I2C2.LEDD.LED1"
> > > > },
> > > > },
> > > >
> > > > It's a property with a string reference to an ACPI non-device node,
> > > > although you can refer to device nodes as well.
> > >
> > > This example is missing the definition of LED0 or LED1 from which it
> > > would be clear that they are data nodes (or at least one of them is a
> > > data node).
> >
> > Ok, perhaps this one could work as a complete example, with a single
> > reference:
> >
> > Package ()
> > {
> > "mipi-img-flash-leds", "\\_SB.PCI0.I2C2.LEDD.LED0",
> > }
> >
> > Device (LEDD)
> > {
> > Name (_DSD, Package () // _DSD: Device-Specific Data
> > {
> > ToUUID ("dbb8e3e6-5886-4ba6-8795-1319f52a966b"), /* Hierarchical Data Extension */,
> > Package ()
> > {
> > Package ()
> > {
> > "mipi-img-flash-led-0",
> > "LED0",
> > }
> > },
> > })
> > Name (LED0, Package () // _DSD: Device-Specific Data
> > {
> > ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */,
> > Package ()
> > {
> > Package ()
> > {
> > "mipi-img-max-current",
> > 1000000,
> > }
> > }
> > })
> > }
> >
>
> This works, thanks!
>
> > >
> > > Also I'm kind of wondering about the "reference with arguments" part
> > > which seems to work differently depending on whether the reference is
> > > represented by a string or by a reference object.
> >
> > Yes. With (device) reference objects, it is possible currently to refer to
> > subnodes with the _DSD data extension child names of those nodes. This is
> > not done with string references as 1) any node can already be referenced so
> > there's no need to and 2) as node references are strings already, it's not
> > possible to distinguish node string references from _DSD data node names.
> > E.g.
> >
> > "\\_SB.I2C0.LED0", "LED1"
> >
> > ^ ACPI object name or _DSD data node name?
> >
>
> Has this behavior been documented anywhere? Or is there any
> expectation to see anything like this shipping in production platform
> firmware?
Good question. Support for this was added by commit 4eb0c3bf5ee52f . AFAIR
it was intended to use this in DisCo for Imaging but after review (in a
rather liberal sense of the term) it was decided to use string-only
references, as in this patch.
I'm not aware of anyone needing this. They've been there for about five
years but I'd guess someone would complain if it stops working for them.
>
> If any of the above isn't the case, I would be inclined to simply
> remove this special case and make both the "object reference" and
> "string" cases work in the same way and if someone needs to refer to a
> data node, they will just need to use a string (in which case it will
> be the only option).
Works for me.
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v8 02/10] ACPI: property: Parse data node string references in properties
2023-05-23 11:43 ` Sakari Ailus
@ 2023-05-23 13:40 ` Rafael J. Wysocki
0 siblings, 0 replies; 36+ messages in thread
From: Rafael J. Wysocki @ 2023-05-23 13:40 UTC (permalink / raw)
To: Sakari Ailus
Cc: Rafael J. Wysocki, linux-acpi, linux-media, andriy.shevchenko,
heikki.krogerus
Hi Sakari,
On Tue, May 23, 2023 at 1:43 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Rafael,
>
> On Tue, May 23, 2023 at 01:21:12PM +0200, Rafael J. Wysocki wrote:
> > On Mon, May 22, 2023 at 10:35 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > > On Mon, May 22, 2023 at 06:38:37PM +0200, Rafael J. Wysocki wrote:
> > > > On Mon, May 22, 2023 at 6:28 PM Sakari Ailus
> > > > <sakari.ailus@linux.intel.com> wrote:
[cut]
> > > >
> > > > Also I'm kind of wondering about the "reference with arguments" part
> > > > which seems to work differently depending on whether the reference is
> > > > represented by a string or by a reference object.
> > >
> > > Yes. With (device) reference objects, it is possible currently to refer to
> > > subnodes with the _DSD data extension child names of those nodes. This is
> > > not done with string references as 1) any node can already be referenced so
> > > there's no need to and 2) as node references are strings already, it's not
> > > possible to distinguish node string references from _DSD data node names.
> > > E.g.
> > >
> > > "\\_SB.I2C0.LED0", "LED1"
> > >
> > > ^ ACPI object name or _DSD data node name?
> > >
> >
> > Has this behavior been documented anywhere? Or is there any
> > expectation to see anything like this shipping in production platform
> > firmware?
>
> Good question. Support for this was added by commit 4eb0c3bf5ee52f . AFAIR
> it was intended to use this in DisCo for Imaging but after review (in a
> rather liberal sense of the term) it was decided to use string-only
> references, as in this patch.
It looks like this is sort of a placeholder then.
> I'm not aware of anyone needing this. They've been there for about five
> years but I'd guess someone would complain if it stops working for them.
It is also quite straightforward to restore if need be.
> >
> > If any of the above isn't the case, I would be inclined to simply
> > remove this special case and make both the "object reference" and
> > "string" cases work in the same way and if someone needs to refer to a
> > data node, they will just need to use a string (in which case it will
> > be the only option).
>
> Works for me.
OK, I'll make this change then.
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v8 03/10] ACPI: property: Parse _CRS CSI-2 descriptor
2023-03-29 10:09 [PATCH v8 00/10] ACPI _CRS CSI-2 and MIPI DisCo for Imaging support Sakari Ailus
2023-03-29 10:09 ` [PATCH v8 01/10] ACPI: scan: Remove the second DSDT traversal Sakari Ailus
2023-03-29 10:09 ` [PATCH v8 02/10] ACPI: property: Parse data node string references in properties Sakari Ailus
@ 2023-03-29 10:09 ` Sakari Ailus
2023-05-15 16:45 ` Rafael J. Wysocki
2023-03-29 10:09 ` [PATCH v8 04/10] device property: Add SOFTWARE_NODE() macro for defining software nodes Sakari Ailus
` (6 subsequent siblings)
9 siblings, 1 reply; 36+ messages in thread
From: Sakari Ailus @ 2023-03-29 10:09 UTC (permalink / raw)
To: linux-acpi; +Cc: linux-media, rafael, andriy.shevchenko, heikki.krogerus
Parse newly added ACPI _CRS CSI-2 descriptor for CSI-2 and camera
configuration, associate it with appropriate devices and allocate memory for
software nodes needed to create a DT-like data structure for drivers.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/acpi/Makefile | 2 +-
drivers/acpi/internal.h | 27 ++++
drivers/acpi/mipi.c | 343 ++++++++++++++++++++++++++++++++++++++++
drivers/acpi/scan.c | 50 ++++--
include/acpi/acpi_bus.h | 11 ++
5 files changed, 418 insertions(+), 15 deletions(-)
create mode 100644 drivers/acpi/mipi.c
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index a5b649e71ab1..a98fa1bc1554 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -37,7 +37,7 @@ acpi-$(CONFIG_ACPI_SLEEP) += proc.o
# ACPI Bus and Device Drivers
#
acpi-y += bus.o glue.o
-acpi-y += scan.o
+acpi-y += scan.o mipi.o
acpi-y += resource.o
acpi-y += acpi_processor.o
acpi-y += processor_core.o
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 06ad497067ac..aa5f9c69dbbe 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -102,6 +102,24 @@ struct acpi_device_bus_id {
struct list_head node;
};
+/* Context for scanning ACPI device nodes for _CRS CSI2 resource descriptors */
+struct acpi_scan_context_csi2 {
+ struct list_head crs_csi2_head;
+ size_t handle_count;
+};
+
+/**
+ * struct acpi_scan_context - Context for scanning ACPI devices
+ * @postponed_head: The list head of the postponed ACPI handles
+ * @device: The first encountered device, typically the root of the scanned tree
+ * @csi2: Context for scanning _CRS CSI2 resource descriptors
+ */
+struct acpi_scan_context {
+ struct list_head postponed_head;
+ struct acpi_device *device;
+ struct acpi_scan_context_csi2 csi2;
+};
+
void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
int type, void (*release)(struct device *));
int acpi_tie_acpi_dev(struct acpi_device *adev);
@@ -282,4 +300,13 @@ void acpi_init_lpit(void);
static inline void acpi_init_lpit(void) { }
#endif
+/*--------------------------------------------------------------------------
+ ACPI _CRS CSI2 and MIPI DisCo for Imaging conversion
+ -------------------------------------------------------------------------- */
+
+void acpi_crs_csi2_swnodes_del_free(void);
+void acpi_bus_scan_check_crs_csi2(acpi_handle handle, struct acpi_scan_context *ctx);
+void acpi_bus_scan_crs_csi2_release(struct list_head *crs_csi2_handles);
+void acpi_bus_scan_crs_csi2(struct acpi_scan_context_csi2 *ctx);
+
#endif /* _ACPI_INTERNAL_H_ */
diff --git a/drivers/acpi/mipi.c b/drivers/acpi/mipi.c
new file mode 100644
index 000000000000..d719c879eb83
--- /dev/null
+++ b/drivers/acpi/mipi.c
@@ -0,0 +1,343 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * MIPI DisCo for Imaging support.
+ *
+ * Support MIPI DisCo for Imaging by parsing ACPI _CRS CSI2 records and DisCo
+ * for Imaging data structures.
+ *
+ * Also see <URL:https://www.mipi.org/specifications/mipi-disco-imaging>.
+ *
+ * Copyright (C) 2023 Intel Corporation
+ */
+
+#include <linux/acpi.h>
+#include <linux/limits.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/overflow.h>
+#include <linux/types.h>
+#include <linux/slab.h>
+#include <linux/sort.h>
+#include <linux/string.h>
+
+#include "internal.h"
+
+/* Temporary ACPI device handle to software node data structure mapping */
+struct crs_csi2_swnodes {
+ struct list_head list;
+ acpi_handle handle;
+ struct acpi_device_software_nodes *ads;
+};
+
+static LIST_HEAD(crs_csi2_swnodes);
+
+static void crs_csi2_swnode_del_free(struct crs_csi2_swnodes *swnodes)
+{
+ list_del(&swnodes->list);
+ kfree(swnodes);
+}
+
+void acpi_crs_csi2_swnodes_del_free(void)
+{
+ struct crs_csi2_swnodes *swnodes, *swnodes_tmp;
+
+ list_for_each_entry_safe(swnodes, swnodes_tmp, &crs_csi2_swnodes, list)
+ crs_csi2_swnode_del_free(swnodes);
+}
+
+/*
+ * Description of a _CRS CSI2 resource descriptor before software node creation
+ */
+struct crs_csi2_instance {
+ struct list_head list;
+ struct acpi_resource_csi2_serialbus csi2;
+ acpi_handle remote_handle;
+ char remote_name[];
+};
+
+/* Temporary list of ACPI device nodes with _CRS CSI2 resource descriptors */
+struct crs_csi2 {
+ struct list_head list;
+ acpi_handle handle;
+ struct list_head buses;
+};
+
+/*
+ * Context for collecting _CRS CSI2 resource descriptors during ACPI namespace
+ * walk
+ */
+struct scan_check_crs_csi2_context {
+ struct list_head res_head;
+ acpi_handle handle;
+ size_t handle_count;
+};
+
+/* Scan a single CSI2 resource descriptor in _CRS. */
+static acpi_status scan_check_crs_csi2_instance(struct acpi_resource *res,
+ void *context)
+{
+ struct scan_check_crs_csi2_context *inst_context = context;
+ struct acpi_resource_csi2_serialbus *csi2;
+ struct crs_csi2_instance *inst;
+ acpi_handle remote_handle;
+
+ if (res->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
+ return AE_OK;
+
+ csi2 = &res->data.csi2_serial_bus;
+
+ if (csi2->type != ACPI_RESOURCE_SERIAL_TYPE_CSI2)
+ return AE_OK;
+
+ if (!csi2->resource_source.string_length) {
+ acpi_handle_debug(inst_context->handle,
+ "invalid resource source string length\n");
+ return AE_OK;
+ }
+
+ if (ACPI_FAILURE(acpi_get_handle(NULL, csi2->resource_source.string_ptr,
+ &remote_handle))) {
+ acpi_handle_warn(inst_context->handle,
+ "cannot get handle for %s\n",
+ csi2->resource_source.string_ptr);
+ return AE_OK;
+ }
+
+ /*
+ * Allocate space to store the _CRS CSI2 entry and its data and add it
+ * to the list.
+ */
+ inst = kmalloc(struct_size(inst, remote_name, csi2->resource_source.string_length),
+ GFP_KERNEL);
+ if (!inst)
+ return AE_OK;
+
+ inst->csi2 = *csi2;
+ strscpy(inst->remote_name, csi2->resource_source.string_ptr,
+ csi2->resource_source.string_length);
+ inst->csi2.resource_source.string_ptr = inst->remote_name;
+ inst->remote_handle = remote_handle;
+
+ list_add(&inst->list, &inst_context->res_head);
+
+ inst_context->handle_count++;
+
+ return AE_OK;
+}
+
+/*
+ * Find all devices with _CRS CSI2 resource descriptors and collect them
+ * into a list for later use.
+ */
+void acpi_bus_scan_check_crs_csi2(acpi_handle handle, struct acpi_scan_context *ctx)
+{
+ struct scan_check_crs_csi2_context inst_context = {
+ .handle = handle,
+ .res_head = LIST_HEAD_INIT(inst_context.res_head),
+ };
+ struct crs_csi2 *csi2;
+
+ acpi_walk_resources(handle, METHOD_NAME__CRS,
+ scan_check_crs_csi2_instance, &inst_context);
+
+ if (list_empty(&inst_context.res_head))
+ return;
+
+ /*
+ * Found entry, so allocate memory for it, fill it and add it to the
+ * list.
+ */
+ csi2 = kmalloc(sizeof(*csi2), GFP_KERNEL);
+ if (!csi2)
+ return; /* There's nothing we really can do about this. */
+
+ csi2->handle = handle;
+ list_replace(&inst_context.res_head, &csi2->buses);
+ list_add(&csi2->list, &ctx->csi2.crs_csi2_head);
+
+ /* This handle plus remote handles in _CRS CSI2 resource descriptors */
+ ctx->csi2.handle_count += 1 + inst_context.handle_count;
+}
+
+struct acpi_handle_ref {
+ acpi_handle handle;
+ unsigned int count;
+};
+
+#define NO_CSI2_PORT (UINT_MAX - 1)
+
+static int crs_handle_cmp(const void *__a, const void *__b)
+{
+ const struct acpi_handle_ref *a = __a, *b = __b;
+
+ return a->handle < b->handle ? -1 : a->handle > b->handle;
+}
+
+/*
+ * Release entries in temporary storage of ACPI device nodes with _CRS CSI2
+ * resource descriptors.
+ */
+void acpi_bus_scan_crs_csi2_release(struct list_head *crs_csi2_handles)
+{
+ struct crs_csi2 *csi2, *csi2_tmp;
+
+ list_for_each_entry_safe(csi2, csi2_tmp, crs_csi2_handles, list) {
+ struct crs_csi2_instance *inst, *inst_tmp;
+
+ list_for_each_entry_safe(inst, inst_tmp, &csi2->buses, list) {
+ list_del(&inst->list);
+ kfree(inst);
+ }
+
+ list_del(&csi2->list);
+ kfree(csi2);
+ }
+}
+
+/*
+ * Allocate memory and set up software nodes for an ACPI device with given
+ * number of CSI-2 ports.
+ */
+static void acpi_crs_csi2_alloc_fill_swnodes(size_t ports_count, acpi_handle handle)
+{
+ struct acpi_device_software_nodes *ads;
+ struct crs_csi2_swnodes *swnodes;
+ size_t alloc_size;
+ unsigned int i;
+ bool overflow;
+ void *end;
+
+ /*
+ * Allocate memory for ports, node pointers (number of nodes +
+ * 1 (guardian), nodes (root + number of ports * 2 (because for
+ * every port there is an endpoint)).
+ */
+ overflow = check_mul_overflow(sizeof(*ads->ports) +
+ sizeof(*ads->nodes) * 2 +
+ sizeof(*ads->nodeptrs) * 2,
+ ports_count, &alloc_size);
+ overflow = overflow ||
+ check_add_overflow(sizeof(*ads) + sizeof(*ads->nodes) +
+ sizeof(*ads->nodeptrs) * 2,
+ alloc_size, &alloc_size);
+ if (overflow) {
+ acpi_handle_warn(handle,
+ "too many _CRS CSI2 resource handles (%zu)",
+ ports_count);
+ return;
+ }
+
+ swnodes = kzalloc(sizeof(*swnodes), GFP_KERNEL);
+ ads = kmalloc(alloc_size, GFP_KERNEL);
+ if (!swnodes || !ads)
+ goto out_free;
+
+ ads->ports = (void *)(ads + 1);
+ ads->nodes = (void *)(ads->ports + ports_count);
+ ads->nodeptrs = (void *)(ads->nodes + ports_count * 2 + 1);
+ end = ads->nodeptrs + ports_count * 2 + 2;
+ if (WARN_ON((void *)ads + alloc_size != end))
+ goto out_free;
+
+ ads->num_ports = ports_count;
+ for (i = 0; i < ports_count * 2 + 1; i++)
+ ads->nodeptrs[i] = &ads->nodes[i];
+ ads->nodeptrs[i] = NULL;
+ for (i = 0; i < ports_count; i++)
+ ads->ports[i].port_nr = NO_CSI2_PORT;
+ swnodes->handle = handle;
+ swnodes->ads = ads;
+ list_add(&swnodes->list, &crs_csi2_swnodes);
+
+ return;
+
+out_free:
+ kfree(swnodes);
+ kfree(ads);
+ acpi_handle_debug(handle, "cannot allocate for %zu software nodes\n",
+ ports_count);
+}
+
+/**
+ * acpi_bus_scan_crs_csi2 - Construct software nodes out of ACPI _CRS CSI2
+ * resource descriptors
+ * @ctx: ACPI _CRS CSI2 context, gathered during tree walk earlier
+ *
+ * This function does a number of things:
+ *
+ * 1. Count how many references to other devices _CRS CSI-2 instances have in
+ * total.
+ *
+ * 2. Count the number of references to other devices for each _CRS CSI-2
+ * instance.
+ *
+ * 3. Allocate memory for swnodes each ACPI device requires later on, and
+ * generate a list of such allocations.
+ *
+ * Note that struct acpi_device may not be available yet at this time.
+ *
+ * acpi_scan_lock in scan.c must be held when calling this function.
+ */
+void acpi_bus_scan_crs_csi2(struct acpi_scan_context_csi2 *ctx)
+{
+ struct acpi_handle_ref *handle_refs;
+ struct acpi_handle_ref *this = NULL;
+ size_t this_count;
+ unsigned int curr = 0;
+ struct crs_csi2 *csi2;
+
+ /* No handles? Bail out here. */
+ if (!ctx->handle_count)
+ return;
+
+ handle_refs = kcalloc(ctx->handle_count + 1, sizeof(*handle_refs),
+ GFP_KERNEL);
+ if (!handle_refs) {
+ pr_debug("no memory for %zu handle refs\n",
+ ctx->handle_count + 1);
+ return;
+ }
+
+ /* Associate handles to the number of references. */
+ list_for_each_entry(csi2, &ctx->crs_csi2_head, list) {
+ struct crs_csi2_instance *inst;
+ struct acpi_handle_ref *handle_ref;
+
+ handle_ref = &handle_refs[curr++];
+ handle_ref->handle = csi2->handle;
+
+ list_for_each_entry(inst, &csi2->buses, list) {
+ handle_refs[curr].handle = inst->remote_handle;
+ handle_refs[curr].count = 1;
+ handle_ref->count++;
+ curr++;
+ }
+ }
+
+ handle_refs[curr].handle = NULL;
+
+ /* Sort the handles by remote so duplicates canbe easily found. */
+ sort(handle_refs, ctx->handle_count, sizeof(*handle_refs), crs_handle_cmp, NULL);
+
+ /*
+ * Finally count references in each handle, allocate space for device
+ * specific ports, properties and fill the _CRS CSI2 descriptor
+ * originated data.
+ */
+ this = handle_refs;
+ this_count = this->count;
+ for (curr = 1; curr < ctx->handle_count + 1; curr++) {
+ /* Weed out duplicate receiver devices. */
+ if (this->handle == handle_refs[curr].handle) {
+ this_count += handle_refs[curr].count;
+ continue;
+ }
+
+ acpi_crs_csi2_alloc_fill_swnodes(this_count, this->handle);
+
+ this = &handle_refs[curr];
+ this_count = this->count;
+ }
+
+ kfree(handle_refs);
+}
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 280d12e0aa2f..ddf7701a57d0 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -2042,16 +2042,6 @@ struct acpi_postponed_handle {
acpi_handle handle;
};
-/**
- * struct acpi_scan_context - Context for scanning ACPI devices
- * @postponed_head: The list head of the postponed ACPI handles
- * @device: The first encountered device, typically the root of the scanned tree
- */
-struct acpi_scan_context {
- struct list_head postponed_head;
- struct acpi_device *device;
-};
-
/**
* acpi_bus_handle_postpone - Add an ACPI handle to a given postponed list
* @handle: The ACPI handle
@@ -2073,7 +2063,7 @@ static void acpi_bus_handle_postpone(acpi_handle handle,
list_add(&ph->list, head);
}
-static acpi_status acpi_bus_check_add(acpi_handle handle, bool check_dep,
+static acpi_status acpi_bus_check_add(acpi_handle handle, bool first_pass,
struct acpi_scan_context *ctx)
{
struct acpi_device *device = acpi_fetch_acpi_dev(handle);
@@ -2091,8 +2081,11 @@ static acpi_status acpi_bus_check_add(acpi_handle handle, bool check_dep,
if (acpi_device_should_be_hidden(handle))
return AE_OK;
+ if (first_pass)
+ acpi_bus_scan_check_crs_csi2(handle, ctx);
+
/* Bail out if there are dependencies. */
- if (acpi_scan_check_dep(handle, check_dep) > 0) {
+ if (acpi_scan_check_dep(handle, first_pass) > 0) {
acpi_bus_handle_postpone(handle, &ctx->postponed_head);
return AE_CTRL_DEPTH;
}
@@ -2118,10 +2111,10 @@ static acpi_status acpi_bus_check_add(acpi_handle handle, bool check_dep,
}
/*
- * If check_dep is true at this point, the device has no dependencies,
+ * If first_pass is true at this point, the device has no dependencies,
* or the creation of the device object would have been postponed above.
*/
- acpi_add_single_object(&device, handle, type, !check_dep);
+ acpi_add_single_object(&device, handle, type, !first_pass);
if (!device)
return AE_CTRL_DEPTH;
@@ -2146,6 +2139,14 @@ static acpi_status acpi_bus_check_add_2(acpi_handle handle, u32 lvl_not_used,
return acpi_bus_check_add(handle, false, (struct acpi_scan_context *)ctx);
}
+static acpi_status acpi_bus_check_csi2(acpi_handle handle, u32 lvl_not_used,
+ void *ctx, void **unused)
+{
+ acpi_bus_scan_check_crs_csi2(handle, ctx);
+
+ return AE_OK;
+}
+
static void acpi_default_enumeration(struct acpi_device *device)
{
/*
@@ -2466,6 +2467,7 @@ int acpi_bus_scan(acpi_handle handle)
{
struct acpi_scan_context ctx = {
.postponed_head = LIST_HEAD_INIT(ctx.postponed_head),
+ .csi2.crs_csi2_head = LIST_HEAD_INIT(ctx.csi2.crs_csi2_head),
};
struct acpi_postponed_handle *ph, *tmp_ph;
int ret = 0;
@@ -2480,6 +2482,24 @@ int acpi_bus_scan(acpi_handle handle)
goto out_release;
}
+ /*
+ * Check _CRS CSI2 resource descriptors for devices that were not
+ * traversed during the previous pass.
+ */
+ list_for_each_entry_safe(ph, tmp_ph, &ctx.postponed_head, list) {
+ acpi_bus_check_csi2(ph->handle, 0, &ctx, NULL);
+ acpi_walk_namespace(ACPI_TYPE_DEVICE, ph->handle, ACPI_UINT32_MAX,
+ acpi_bus_check_csi2, NULL, (void *)&ctx,
+ NULL);
+ }
+
+ /*
+ * Construct software nodes out of _CRS CSI2 records and release the
+ * _CRS CSI2 records.
+ */
+ acpi_bus_scan_crs_csi2(&ctx.csi2);
+ acpi_bus_scan_crs_csi2_release(&ctx.csi2.crs_csi2_head);
+
acpi_bus_attach(ctx.device, (void *)true);
/*
@@ -2511,6 +2531,8 @@ int acpi_bus_scan(acpi_handle handle)
kfree(ph);
}
+ acpi_crs_csi2_swnodes_del_free();
+
return ret;
}
EXPORT_SYMBOL(acpi_bus_scan);
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 57acb895c038..e4e8faac9212 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -360,6 +360,17 @@ struct acpi_device_data {
struct acpi_gpio_mapping;
+struct acpi_device_software_node_port {
+ unsigned int port_nr;
+};
+
+struct acpi_device_software_nodes {
+ struct acpi_device_software_node_port *ports;
+ struct software_node *nodes;
+ const struct software_node **nodeptrs;
+ unsigned int num_ports;
+};
+
/* Device */
struct acpi_device {
u32 pld_crc;
--
2.30.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v8 03/10] ACPI: property: Parse _CRS CSI-2 descriptor
2023-03-29 10:09 ` [PATCH v8 03/10] ACPI: property: Parse _CRS CSI-2 descriptor Sakari Ailus
@ 2023-05-15 16:45 ` Rafael J. Wysocki
2023-05-16 8:57 ` Sakari Ailus
0 siblings, 1 reply; 36+ messages in thread
From: Rafael J. Wysocki @ 2023-05-15 16:45 UTC (permalink / raw)
To: linux-acpi, Sakari Ailus
Cc: linux-media, rafael, andriy.shevchenko, heikki.krogerus
On Wednesday, March 29, 2023 12:09:44 PM CEST Sakari Ailus wrote:
> Parse newly added ACPI _CRS CSI-2 descriptor for CSI-2 and camera
> configuration, associate it with appropriate devices and allocate memory for
> software nodes needed to create a DT-like data structure for drivers.
It occurred to me, that there would be so many things I would like to change
in this patch, so it would be better to create my own version of it, which
is appended.
It is based on
https://patchwork.kernel.org/project/linux-acpi/patch/2694293.mvXUDI8C0e@kreacher/
that has just been posted.
IIUC, the idea is to extract the ACPI handle for each resource source in every
_CRS CSI-2 resource descriptor and count how many times each handle appears in
a CSI-2 context, either because it is referenced from a _CRS CSI-2 resource
descriptor (as a "resource source"), or because its device object has CSI-2
resource descriptors in _CRS.
This allows a set of software nodes to be allocated for each of these handles.
If I got that totally wrong, please let me know. Otherwise, I will rework the
remaining patches in the series to match this one.
Thanks!
---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: [PATCH] ACPI: scan: Parse _CRS CSI-2 descriptors for all devices
Parse ACPI _CRS CSI-2 resource descriptors defined since ACPI 6.4 (for CSI-2 and
camera configuration) for all device objects in the ACPI namespace, find the
corresponding "remote" device objects for them and allocate memory for software
nodes needed to create a DT-like data structure for drivers (that will be taken
care of by subsequent patches).
Co-developed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/acpi/Makefile | 2
drivers/acpi/internal.h | 7
drivers/acpi/mipi-disco-imaging.c | 313 ++++++++++++++++++++++++++++++++++++++
drivers/acpi/scan.c | 49 ++++-
include/acpi/acpi_bus.h | 11 +
5 files changed, 372 insertions(+), 10 deletions(-)
create mode 100644 drivers/acpi/mipi-disco-imaging.c
Index: linux-pm/drivers/acpi/Makefile
===================================================================
--- linux-pm.orig/drivers/acpi/Makefile
+++ linux-pm/drivers/acpi/Makefile
@@ -37,7 +37,7 @@ acpi-$(CONFIG_ACPI_SLEEP) += proc.o
# ACPI Bus and Device Drivers
#
acpi-y += bus.o glue.o
-acpi-y += scan.o
+acpi-y += scan.o mipi-disco-imaging.o
acpi-y += resource.o
acpi-y += acpi_processor.o
acpi-y += processor_core.o
Index: linux-pm/drivers/acpi/internal.h
===================================================================
--- linux-pm.orig/drivers/acpi/internal.h
+++ linux-pm/drivers/acpi/internal.h
@@ -282,4 +282,11 @@ void acpi_init_lpit(void);
static inline void acpi_init_lpit(void) { }
#endif
+/*--------------------------------------------------------------------------
+ ACPI _CRS CSI-2 and MIPI DisCo for Imaging
+ -------------------------------------------------------------------------- */
+
+void acpi_mipi_check_crs_csi2(acpi_handle handle);
+void acpi_mipi_scan_crs_csi2(void);
+
#endif /* _ACPI_INTERNAL_H_ */
Index: linux-pm/drivers/acpi/mipi-disco-imaging.c
===================================================================
--- /dev/null
+++ linux-pm/drivers/acpi/mipi-disco-imaging.c
@@ -0,0 +1,313 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * MIPI DisCo for Imaging support.
+ *
+ * Copyright (C) 2023 Intel Corporation
+ *
+ * Support MIPI DisCo for Imaging by parsing ACPI _CRS CSI-2 records defined in
+ * Section 6.4.3.8.2.4 "Camera Serial Interface (CSI-2) Connection Resource
+ * Descriptor" of ACPI 6.5.
+ *
+ * The implementation looks for the information in the ACPI namespace (CSI-2
+ * resource descriptors in _CRS) and constructs software nodes that are
+ * compatible with Documentation/firmware-guide/acpi/dsd/graph.rst.
+ */
+
+#include <linux/acpi.h>
+#include <linux/limits.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/overflow.h>
+#include <linux/types.h>
+#include <linux/slab.h>
+#include <linux/sort.h>
+#include <linux/string.h>
+
+#include "internal.h"
+
+static LIST_HEAD(acpi_mipi_crs_csi2_list);
+
+/*
+ * List entry for the list of devices with software nodes allocated in
+ * accordance with the information from _CRS CSI-2 device desciptiors.
+ */
+struct crs_csi2_dev_swnodes {
+ struct list_head entry;
+ acpi_handle handle;
+ struct acpi_device_software_nodes *swnodes;
+};
+
+static LIST_HEAD(crs_csi2_swnodes);
+
+static void acpi_mipi_data_tag(acpi_handle handle, void *context)
+{
+}
+
+/* Connection data extracted from one _CRS CSI-2 resource descriptor. */
+struct crs_csi2_connection {
+ struct list_head entry;
+ struct acpi_resource_csi2_serialbus csi2_data;
+ acpi_handle remote_handle;
+ char remote_name[];
+};
+
+/* Data extracted from _CRS CSI-2 resource descriptors for one device. */
+struct crs_csi2 {
+ struct list_head entry;
+ acpi_handle handle;
+ struct list_head connections;
+ u32 port_count;
+};
+
+static acpi_status csi2_resource_extract(struct acpi_resource *res, void *context)
+{
+ struct crs_csi2 *csi2 = context;
+ struct acpi_resource_csi2_serialbus *csi2_res;
+ struct acpi_resource_source *csi2_res_src;
+ u16 csi2_res_src_length;
+ struct crs_csi2_connection *conn;
+ acpi_handle remote_handle;
+
+ if (res->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
+ return AE_OK;
+
+ csi2_res = &res->data.csi2_serial_bus;
+
+ if (csi2_res->type != ACPI_RESOURCE_SERIAL_TYPE_CSI2)
+ return AE_OK;
+
+ csi2_res_src = &csi2_res->resource_source;
+ if (ACPI_FAILURE(acpi_get_handle(NULL, csi2_res_src->string_ptr,
+ &remote_handle))) {
+ acpi_handle_debug(csi2->handle,
+ "unable to find resource source\n");
+ return AE_OK;
+ }
+ csi2_res_src_length = csi2_res_src->string_length;
+ if (!csi2_res_src_length) {
+ acpi_handle_debug(csi2->handle,
+ "invalid resource source string length\n");
+ return AE_OK;
+ }
+
+ conn = kmalloc(struct_size(conn, remote_name, csi2_res_src_length + 1),
+ GFP_KERNEL);
+ if (!conn)
+ return AE_OK;
+
+ conn->csi2_data = *csi2_res;
+ strscpy(conn->remote_name, csi2_res_src->string_ptr, csi2_res_src_length);
+ conn->csi2_data.resource_source.string_ptr = conn->remote_name;
+ conn->remote_handle = remote_handle;
+
+ list_add(&conn->entry, &csi2->connections);
+
+ return AE_OK;
+}
+
+static struct crs_csi2 *create_crs_csi2_entry(acpi_handle handle)
+{
+ struct crs_csi2 *csi2_entry;
+
+ csi2_entry = kzalloc(sizeof(*csi2_entry), GFP_KERNEL);
+ if (!csi2_entry)
+ return NULL;
+
+ csi2_entry->handle = handle;
+ INIT_LIST_HEAD(&csi2_entry->connections);
+ csi2_entry->port_count = 1;
+
+ if (ACPI_FAILURE(acpi_attach_data(handle, acpi_mipi_data_tag, csi2_entry))) {
+ kfree(csi2_entry);
+ return NULL;
+ }
+
+ return csi2_entry;
+}
+
+/**
+ * acpi_mipi_check_crs_csi2 - Look for devices with _CRS CSI-2 resources
+ * @handle: ACPI namespace walk starting point.
+ *
+ * Find all devices with _CRS CSI-2 resource descriptors in the ACPI namespace
+ * branch starting at @handle and collect them into a list.
+ */
+void acpi_mipi_check_crs_csi2(acpi_handle handle)
+{
+ struct crs_csi2 csi2 = {
+ .handle = handle,
+ .connections = LIST_HEAD_INIT(csi2.connections),
+ };
+ struct crs_csi2 *csi2_entry;
+
+ /*
+ * Avoid creating _CRS CSI-2 list entries for devices without any CSI-2
+ * resource descriptions in _CRS to reduce overhead.
+ */
+ acpi_walk_resources(handle, METHOD_NAME__CRS, csi2_resource_extract, &csi2);
+ if (list_empty(&csi2.connections))
+ return;
+
+ /*
+ * Create a _CRS CSI-2 entry to store the extracted connection
+ * information and add it to the global list.
+ */
+ csi2_entry = create_crs_csi2_entry(handle);
+ if (!csi2_entry)
+ return; /* There's nothing we really can do about this. */
+
+ list_replace(&csi2.connections, &csi2_entry->connections);
+ list_add(&csi2_entry->entry, &acpi_mipi_crs_csi2_list);
+}
+
+#define NO_CSI2_PORT (UINT_MAX - 1)
+
+static void alloc_fill_crs_csi2_swnodes(acpi_handle handle, size_t port_count)
+{
+ struct acpi_device_software_nodes *swnodes;
+ struct crs_csi2_dev_swnodes *swnodes_entry;
+ size_t alloc_size;
+ unsigned int i;
+ bool overflow;
+
+ /*
+ * Allocate memory for ports, node pointers (number of nodes +
+ * 1 (guardian), nodes (root + number of ports * 2 (because for
+ * every port there is an endpoint)).
+ */
+ overflow = check_mul_overflow(sizeof(*swnodes->ports) +
+ sizeof(*swnodes->nodes) * 2 +
+ sizeof(*swnodes->nodeptrs) * 2,
+ port_count, &alloc_size);
+ overflow = overflow ||
+ check_add_overflow(sizeof(*swnodes) +
+ sizeof(*swnodes->nodes) +
+ sizeof(*swnodes->nodeptrs) * 2,
+ alloc_size, &alloc_size);
+ if (overflow) {
+ acpi_handle_warn(handle,
+ "too many _CRS CSI-2 resource handles (%zu)",
+ port_count);
+ return;
+ }
+
+ swnodes_entry = kzalloc(sizeof(*swnodes_entry), GFP_KERNEL);
+ swnodes = kmalloc(alloc_size, GFP_KERNEL);
+ if (!swnodes_entry || !swnodes)
+ goto out_free;
+
+ swnodes->ports = (struct acpi_device_software_node_port *)(swnodes + 1);
+ swnodes->nodes = (struct software_node *)(swnodes->ports + port_count);
+ swnodes->nodeptrs = (const struct software_node **)(swnodes->nodes + 1 +
+ 2 * port_count);
+ swnodes->num_ports = port_count;
+
+ for (i = 0; i < 2 * port_count + 1; i++)
+ swnodes->nodeptrs[i] = &swnodes->nodes[i];
+
+ swnodes->nodeptrs[i] = NULL;
+
+ for (i = 0; i < port_count; i++)
+ swnodes->ports[i].port_nr = NO_CSI2_PORT;
+
+ swnodes_entry->handle = handle;
+ swnodes_entry->swnodes = swnodes;
+
+ list_add(&swnodes_entry->entry, &crs_csi2_swnodes);
+
+ return;
+
+out_free:
+ kfree(swnodes_entry);
+ kfree(swnodes);
+ acpi_handle_debug(handle, "unable to allocate for %zu software nodes\n",
+ port_count);
+}
+
+static void acpi_mipi_crs_csi2_release(void)
+{
+ struct crs_csi2 *csi2, *csi2_tmp;
+
+ list_for_each_entry_safe(csi2, csi2_tmp, &acpi_mipi_crs_csi2_list, entry) {
+ struct crs_csi2_connection *conn, *conn_tmp;
+
+ list_for_each_entry_safe(conn, conn_tmp, &csi2->connections,
+ entry) {
+ list_del(&conn->entry);
+ kfree(conn);
+ }
+
+ list_del(&csi2->entry);
+ acpi_detach_data(csi2->handle, acpi_mipi_data_tag);
+ kfree(csi2);
+ }
+}
+
+/**
+ * acpi_mipi_scan_crs_csi2 - Allocate ACPI _CRS CSI-2 software nodes
+ *
+ * Note that this function must be called before any struct acpi_device objects
+ * are bound to any ACPI drivers or scan handlers, so it cannot assume the
+ * existence of struct acpi_device objects for every device present in the ACPI
+ * namespace.
+ *
+ * acpi_scan_lock in scan.c must be held when calling this function.
+ */
+void acpi_mipi_scan_crs_csi2(void)
+{
+ struct crs_csi2 *csi2;
+ LIST_HEAD(aux_list);
+
+ /* Count references to each ACPI handle in the CSI-2 connection graph. */
+ list_for_each_entry(csi2, &acpi_mipi_crs_csi2_list, entry) {
+ struct crs_csi2_connection *conn;
+
+ list_for_each_entry(conn, &csi2->connections, entry) {
+ struct crs_csi2 *remote_csi2;
+ acpi_status status;
+
+ status = acpi_get_data_full(conn->remote_handle,
+ acpi_mipi_data_tag,
+ (void **)&remote_csi2,
+ NULL);
+ if (ACPI_SUCCESS(status) && remote_csi2) {
+ remote_csi2->port_count++;
+ continue;
+ }
+
+ /*
+ * The "remote" device has no _CRS CSI-2 list entry yet,
+ * so create one for it and add it to the list.
+ */
+ remote_csi2 = create_crs_csi2_entry(conn->remote_handle);
+ if (!remote_csi2)
+ continue;
+
+ list_add(&remote_csi2->entry, &aux_list);
+ }
+ }
+ list_splice(&aux_list, &acpi_mipi_crs_csi2_list);
+
+ /* Allocate softwware nodes for representing the CSI-2 information. */
+ list_for_each_entry(csi2, &acpi_mipi_crs_csi2_list, entry)
+ alloc_fill_crs_csi2_swnodes(csi2->handle, csi2->port_count);
+
+ acpi_mipi_crs_csi2_release();
+}
+
+static void crs_csi2_swnode_del_free(struct crs_csi2_dev_swnodes *swnodes)
+{
+ list_del(&swnodes->entry);
+ kfree(swnodes);
+}
+
+/**
+ * acpi_mipi_swnodes_release - Free the list of devices with _CRS CSI-2 swnodes
+ */
+void acpi_mipi_swnodes_release(void)
+{
+ struct crs_csi2_dev_swnodes *sn, *sn_tmp;
+
+ list_for_each_entry_safe(sn, sn_tmp, &crs_csi2_swnodes, entry)
+ crs_csi2_swnode_del_free(sn);
+}
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -363,6 +363,17 @@ struct acpi_device_data {
struct acpi_gpio_mapping;
+struct acpi_device_software_node_port {
+ unsigned int port_nr;
+};
+
+struct acpi_device_software_nodes {
+ struct software_node *nodes;
+ const struct software_node **nodeptrs;
+ struct acpi_device_software_node_port *ports;
+ unsigned int num_ports;
+};
+
/* Device */
struct acpi_device {
u32 pld_crc;
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1970,7 +1970,7 @@ static void acpi_scan_init_hotplug(struc
}
}
-static u32 acpi_scan_check_dep(acpi_handle handle, bool check_dep)
+static u32 acpi_scan_check_dep(acpi_handle handle)
{
struct acpi_handle_list dep_devices;
acpi_status status;
@@ -1983,8 +1983,7 @@ static u32 acpi_scan_check_dep(acpi_hand
* 2. ACPI nodes describing USB ports.
* Still, checking for _HID catches more then just these cases ...
*/
- if (!check_dep || !acpi_has_method(handle, "_DEP") ||
- !acpi_has_method(handle, "_HID"))
+ if (!acpi_has_method(handle, "_DEP") || !acpi_has_method(handle, "_HID"))
return 0;
status = acpi_evaluate_reference(handle, "_DEP", NULL, &dep_devices);
@@ -2029,7 +2028,16 @@ static u32 acpi_scan_check_dep(acpi_hand
return count;
}
-static acpi_status acpi_bus_check_add(acpi_handle handle, bool check_dep,
+static acpi_status acpi_scan_check_crs_csi2_cb(acpi_handle handle,
+ u32 lvl_not_used,
+ void *not_used,
+ void **ret_p_not_used)
+{
+ acpi_mipi_check_crs_csi2(handle);
+ return AE_OK;
+}
+
+static acpi_status acpi_bus_check_add(acpi_handle handle, bool first_pass,
struct acpi_device **adev_p)
{
struct acpi_device *device = acpi_fetch_acpi_dev(handle);
@@ -2047,9 +2055,25 @@ static acpi_status acpi_bus_check_add(ac
if (acpi_device_should_be_hidden(handle))
return AE_OK;
- /* Bail out if there are dependencies. */
- if (acpi_scan_check_dep(handle, check_dep) > 0)
- return AE_CTRL_DEPTH;
+ if (first_pass) {
+ acpi_mipi_check_crs_csi2(handle);
+
+ /* Bail out if there are dependencies. */
+ if (acpi_scan_check_dep(handle) > 0) {
+ /*
+ * The entire CSI-2 connection graph needs to be
+ * extracted before any drivers or scan handlers
+ * are bound to struct device objects, so scan
+ * _CRS CSI-2 resource descriptors for all
+ * devices below the current handle.
+ */
+ acpi_walk_namespace(ACPI_TYPE_DEVICE, handle,
+ ACPI_UINT32_MAX,
+ acpi_scan_check_crs_csi2_cb,
+ NULL, NULL, NULL);
+ return AE_CTRL_DEPTH;
+ }
+ }
fallthrough;
case ACPI_TYPE_ANY: /* for ACPI_ROOT_OBJECT */
@@ -2072,10 +2096,10 @@ static acpi_status acpi_bus_check_add(ac
}
/*
- * If check_dep is true at this point, the device has no dependencies,
+ * If first_pass is true at this point, the device has no dependencies,
* or the creation of the device object would have been postponed above.
*/
- acpi_add_single_object(&device, handle, type, !check_dep);
+ acpi_add_single_object(&device, handle, type, !first_pass);
if (!device)
return AE_CTRL_DEPTH;
@@ -2487,6 +2511,13 @@ int acpi_bus_scan(acpi_handle handle)
if (!device)
return -ENODEV;
+ /*
+ * Allocate ACPI _CRS CSI-2 software nodes using information extracted
+ * from the _CRS CSI-2 resource descriptors during the ACPI namespace
+ * walk above.
+ */
+ acpi_mipi_scan_crs_csi2();
+
acpi_bus_attach(device, (void *)true);
/* Pass 2: Enumerate all of the remaining devices. */
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v8 03/10] ACPI: property: Parse _CRS CSI-2 descriptor
2023-05-15 16:45 ` Rafael J. Wysocki
@ 2023-05-16 8:57 ` Sakari Ailus
2023-05-16 10:12 ` Rafael J. Wysocki
0 siblings, 1 reply; 36+ messages in thread
From: Sakari Ailus @ 2023-05-16 8:57 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: linux-acpi, linux-media, rafael, andriy.shevchenko,
heikki.krogerus
Hi Rafael,
On Mon, May 15, 2023 at 06:45:10PM +0200, Rafael J. Wysocki wrote:
> On Wednesday, March 29, 2023 12:09:44 PM CEST Sakari Ailus wrote:
> > Parse newly added ACPI _CRS CSI-2 descriptor for CSI-2 and camera
> > configuration, associate it with appropriate devices and allocate memory for
> > software nodes needed to create a DT-like data structure for drivers.
>
> It occurred to me, that there would be so many things I would like to change
> in this patch, so it would be better to create my own version of it, which
> is appended.
>
> It is based on
>
> https://patchwork.kernel.org/project/linux-acpi/patch/2694293.mvXUDI8C0e@kreacher/
>
> that has just been posted.
>
> IIUC, the idea is to extract the ACPI handle for each resource source in every
> _CRS CSI-2 resource descriptor and count how many times each handle appears in
> a CSI-2 context, either because it is referenced from a _CRS CSI-2 resource
> descriptor (as a "resource source"), or because its device object has CSI-2
> resource descriptors in _CRS.
Correct.
>
> This allows a set of software nodes to be allocated for each of these handles.
>
> If I got that totally wrong, please let me know. Otherwise, I will rework the
> remaining patches in the series to match this one.
It seems about right. I mostly see renames, moving the code around,
using the existing dependency list and then parsing sub-tree for _CRS CSI-2
objects right from the bus scan callback.
It seems you've also moved the structs from internal.h to what is now
called mipi-disco-imaging.c. They'll be later needed in e.g. scan.c. At
least I'd use names that indicate they're related to scanning the bus:
they're not needed after this is done.
I don't have objections to you reworking the rest, but given the number of
non-trivial changes, will it work after this? :-) I can also do this,
although I would un-do some of the changes in this patch in order to
prepare for the rest (such as moving the structs from internal.h).
See e.g. "ACPI: scan: Generate software nodes based on MIPI DisCo for
Imaging", I think it's the 6th patch.
...
> +/**
> + * acpi_mipi_scan_crs_csi2 - Allocate ACPI _CRS CSI-2 software nodes
> + *
> + * Note that this function must be called before any struct acpi_device objects
> + * are bound to any ACPI drivers or scan handlers, so it cannot assume the
> + * existence of struct acpi_device objects for every device present in the ACPI
> + * namespace.
> + *
> + * acpi_scan_lock in scan.c must be held when calling this function.
> + */
> +void acpi_mipi_scan_crs_csi2(void)
> +{
> + struct crs_csi2 *csi2;
> + LIST_HEAD(aux_list);
> +
> + /* Count references to each ACPI handle in the CSI-2 connection graph. */
> + list_for_each_entry(csi2, &acpi_mipi_crs_csi2_list, entry) {
> + struct crs_csi2_connection *conn;
> +
> + list_for_each_entry(conn, &csi2->connections, entry) {
> + struct crs_csi2 *remote_csi2;
> + acpi_status status;
> +
> + status = acpi_get_data_full(conn->remote_handle,
> + acpi_mipi_data_tag,
> + (void **)&remote_csi2,
> + NULL);
> + if (ACPI_SUCCESS(status) && remote_csi2) {
> + remote_csi2->port_count++;
> + continue;
> + }
> +
> + /*
> + * The "remote" device has no _CRS CSI-2 list entry yet,
> + * so create one for it and add it to the list.
> + */
> + remote_csi2 = create_crs_csi2_entry(conn->remote_handle);
> + if (!remote_csi2)
> + continue;
> +
> + list_add(&remote_csi2->entry, &aux_list);
> + }
> + }
> + list_splice(&aux_list, &acpi_mipi_crs_csi2_list);
> +
> + /* Allocate softwware nodes for representing the CSI-2 information. */
"software"
> + list_for_each_entry(csi2, &acpi_mipi_crs_csi2_list, entry)
> + alloc_fill_crs_csi2_swnodes(csi2->handle, csi2->port_count);
> +
> + acpi_mipi_crs_csi2_release();
> +}
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v8 03/10] ACPI: property: Parse _CRS CSI-2 descriptor
2023-05-16 8:57 ` Sakari Ailus
@ 2023-05-16 10:12 ` Rafael J. Wysocki
2023-05-16 11:09 ` Sakari Ailus
0 siblings, 1 reply; 36+ messages in thread
From: Rafael J. Wysocki @ 2023-05-16 10:12 UTC (permalink / raw)
To: Sakari Ailus
Cc: Rafael J. Wysocki, linux-acpi, linux-media, rafael,
andriy.shevchenko, heikki.krogerus
Hi Sakari,
On Tue, May 16, 2023 at 10:57 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Rafael,
>
> On Mon, May 15, 2023 at 06:45:10PM +0200, Rafael J. Wysocki wrote:
> > On Wednesday, March 29, 2023 12:09:44 PM CEST Sakari Ailus wrote:
> > > Parse newly added ACPI _CRS CSI-2 descriptor for CSI-2 and camera
> > > configuration, associate it with appropriate devices and allocate memory for
> > > software nodes needed to create a DT-like data structure for drivers.
> >
> > It occurred to me, that there would be so many things I would like to change
> > in this patch, so it would be better to create my own version of it, which
> > is appended.
> >
> > It is based on
> >
> > https://patchwork.kernel.org/project/linux-acpi/patch/2694293.mvXUDI8C0e@kreacher/
> >
> > that has just been posted.
> >
> > IIUC, the idea is to extract the ACPI handle for each resource source in every
> > _CRS CSI-2 resource descriptor and count how many times each handle appears in
> > a CSI-2 context, either because it is referenced from a _CRS CSI-2 resource
> > descriptor (as a "resource source"), or because its device object has CSI-2
> > resource descriptors in _CRS.
>
> Correct.
>
> >
> > This allows a set of software nodes to be allocated for each of these handles.
> >
> > If I got that totally wrong, please let me know. Otherwise, I will rework the
> > remaining patches in the series to match this one.
>
> It seems about right. I mostly see renames, moving the code around,
> using the existing dependency list and then parsing sub-tree for _CRS CSI-2
> objects right from the bus scan callback.
>
> It seems you've also moved the structs from internal.h to what is now
> called mipi-disco-imaging.c.
No, I haven't moved anything in this direction, I've just dropped them.
They can be added in the next patches if needed.
> They'll be later needed in e.g. scan.c. At
> least I'd use names that indicate they're related to scanning the bus:
> they're not needed after this is done.
>
> I don't have objections to you reworking the rest, but given the number of
> non-trivial changes, will it work after this? :-)
Probably not right from the start, but after some minor adjustments it
should work, unless I've missed something significant.
> I can also do this, although I would un-do some of the changes in this patch in order to
> prepare for the rest (such as moving the structs from internal.h).
>
> See e.g. "ACPI: scan: Generate software nodes based on MIPI DisCo for
> Imaging", I think it's the 6th patch.
I will.
Thanks!
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v8 03/10] ACPI: property: Parse _CRS CSI-2 descriptor
2023-05-16 10:12 ` Rafael J. Wysocki
@ 2023-05-16 11:09 ` Sakari Ailus
0 siblings, 0 replies; 36+ messages in thread
From: Sakari Ailus @ 2023-05-16 11:09 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael J. Wysocki, linux-acpi, linux-media, andriy.shevchenko,
heikki.krogerus
Hi Rafael,
On Tue, May 16, 2023 at 12:12:13PM +0200, Rafael J. Wysocki wrote:
> Hi Sakari,
>
> On Tue, May 16, 2023 at 10:57 AM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Rafael,
> >
> > On Mon, May 15, 2023 at 06:45:10PM +0200, Rafael J. Wysocki wrote:
> > > On Wednesday, March 29, 2023 12:09:44 PM CEST Sakari Ailus wrote:
> > > > Parse newly added ACPI _CRS CSI-2 descriptor for CSI-2 and camera
> > > > configuration, associate it with appropriate devices and allocate memory for
> > > > software nodes needed to create a DT-like data structure for drivers.
> > >
> > > It occurred to me, that there would be so many things I would like to change
> > > in this patch, so it would be better to create my own version of it, which
> > > is appended.
> > >
> > > It is based on
> > >
> > > https://patchwork.kernel.org/project/linux-acpi/patch/2694293.mvXUDI8C0e@kreacher/
> > >
> > > that has just been posted.
> > >
> > > IIUC, the idea is to extract the ACPI handle for each resource source in every
> > > _CRS CSI-2 resource descriptor and count how many times each handle appears in
> > > a CSI-2 context, either because it is referenced from a _CRS CSI-2 resource
> > > descriptor (as a "resource source"), or because its device object has CSI-2
> > > resource descriptors in _CRS.
> >
> > Correct.
> >
> > >
> > > This allows a set of software nodes to be allocated for each of these handles.
> > >
> > > If I got that totally wrong, please let me know. Otherwise, I will rework the
> > > remaining patches in the series to match this one.
> >
> > It seems about right. I mostly see renames, moving the code around,
> > using the existing dependency list and then parsing sub-tree for _CRS CSI-2
> > objects right from the bus scan callback.
> >
> > It seems you've also moved the structs from internal.h to what is now
> > called mipi-disco-imaging.c.
>
> No, I haven't moved anything in this direction, I've just dropped them.
Ah, I think the struct was added to scan.c by the earlier patch. Indeed, by
doing the _CRS CSI2 scanning right from the device scanning callback, it's
probably possible to omit these.
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v8 04/10] device property: Add SOFTWARE_NODE() macro for defining software nodes
2023-03-29 10:09 [PATCH v8 00/10] ACPI _CRS CSI-2 and MIPI DisCo for Imaging support Sakari Ailus
` (2 preceding siblings ...)
2023-03-29 10:09 ` [PATCH v8 03/10] ACPI: property: Parse _CRS CSI-2 descriptor Sakari Ailus
@ 2023-03-29 10:09 ` Sakari Ailus
2023-03-29 10:09 ` [PATCH v8 05/10] ACPI: property: Prepare generating swnodes for ACPI and DisCo for Imaging Sakari Ailus
` (5 subsequent siblings)
9 siblings, 0 replies; 36+ messages in thread
From: Sakari Ailus @ 2023-03-29 10:09 UTC (permalink / raw)
To: linux-acpi; +Cc: linux-media, rafael, andriy.shevchenko, heikki.krogerus
Add SOFTWARE_NODE() macro in order to make defining software nodes look
nicer. This is analogous to different PROPERTY_ENTRY_*() macros for
defining properties.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
include/linux/property.h | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/include/linux/property.h b/include/linux/property.h
index 0a29db15ff34..b2562f512c49 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -477,6 +477,13 @@ struct software_node {
const struct property_entry *properties;
};
+#define SOFTWARE_NODE(_name_, _properties_, _parent_) \
+ (struct software_node) { \
+ .name = _name_, \
+ .properties = _properties_, \
+ .parent = _parent_, \
+ }
+
bool is_software_node(const struct fwnode_handle *fwnode);
const struct software_node *
to_software_node(const struct fwnode_handle *fwnode);
--
2.30.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v8 05/10] ACPI: property: Prepare generating swnodes for ACPI and DisCo for Imaging
2023-03-29 10:09 [PATCH v8 00/10] ACPI _CRS CSI-2 and MIPI DisCo for Imaging support Sakari Ailus
` (3 preceding siblings ...)
2023-03-29 10:09 ` [PATCH v8 04/10] device property: Add SOFTWARE_NODE() macro for defining software nodes Sakari Ailus
@ 2023-03-29 10:09 ` Sakari Ailus
2023-03-29 14:39 ` Andy Shevchenko
2023-05-17 10:53 ` Rafael J. Wysocki
2023-03-29 10:09 ` [PATCH v8 06/10] ACPI: scan: Generate software nodes based on MIPI " Sakari Ailus
` (4 subsequent siblings)
9 siblings, 2 replies; 36+ messages in thread
From: Sakari Ailus @ 2023-03-29 10:09 UTC (permalink / raw)
To: linux-acpi; +Cc: linux-media, rafael, andriy.shevchenko, heikki.krogerus
Prepare generating software nodes for information parsed from ACPI _CRS for
CSI-2 as well as MIPI DisCo for Imaging spec. The software nodes are
compliant with existing ACPI or DT definitions and are parsed by relevant
drivers without changes.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/acpi/internal.h | 3 +
drivers/acpi/mipi.c | 358 +++++++++++++++++++++++++++++++++++++++-
drivers/acpi/scan.c | 21 ++-
include/acpi/acpi_bus.h | 70 ++++++++
4 files changed, 449 insertions(+), 3 deletions(-)
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index aa5f9c69dbbe..1634a177c75e 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -304,9 +304,12 @@ static inline void acpi_init_lpit(void) { }
ACPI _CRS CSI2 and MIPI DisCo for Imaging conversion
-------------------------------------------------------------------------- */
+#define MIPI_IMG_PORT_PREFIX "mipi-img-port-"
+
void acpi_crs_csi2_swnodes_del_free(void);
void acpi_bus_scan_check_crs_csi2(acpi_handle handle, struct acpi_scan_context *ctx);
void acpi_bus_scan_crs_csi2_release(struct list_head *crs_csi2_handles);
void acpi_bus_scan_crs_csi2(struct acpi_scan_context_csi2 *ctx);
+void acpi_init_swnodes(struct acpi_device *device);
#endif /* _ACPI_INTERNAL_H_ */
diff --git a/drivers/acpi/mipi.c b/drivers/acpi/mipi.c
index d719c879eb83..03079f78bd2c 100644
--- a/drivers/acpi/mipi.c
+++ b/drivers/acpi/mipi.c
@@ -3,7 +3,8 @@
* MIPI DisCo for Imaging support.
*
* Support MIPI DisCo for Imaging by parsing ACPI _CRS CSI2 records and DisCo
- * for Imaging data structures.
+ * for Imaging data structures and generating nodes and properties using
+ * software nodes compliant with DT definitions of the similar scope.
*
* Also see <URL:https://www.mipi.org/specifications/mipi-disco-imaging>.
*
@@ -20,6 +21,8 @@
#include <linux/sort.h>
#include <linux/string.h>
+#include <media/v4l2-fwnode.h>
+
#include "internal.h"
/* Temporary ACPI device handle to software node data structure mapping */
@@ -31,6 +34,18 @@ struct crs_csi2_swnodes {
static LIST_HEAD(crs_csi2_swnodes);
+/* Obtain pre-allocated software nodes for an ACPI device handle */
+static struct acpi_device_software_nodes *crs_csi2_swnode_get(acpi_handle handle)
+{
+ struct crs_csi2_swnodes *swnodes;
+
+ list_for_each_entry(swnodes, &crs_csi2_swnodes, list)
+ if (swnodes->handle == handle)
+ return swnodes->ads;
+
+ return NULL;
+}
+
static void crs_csi2_swnode_del_free(struct crs_csi2_swnodes *swnodes)
{
list_del(&swnodes->list);
@@ -166,6 +181,35 @@ struct acpi_handle_ref {
#define NO_CSI2_PORT (UINT_MAX - 1)
+/*
+ * Return next free entry in ports array of a software nodes related to an ACPI
+ * device.
+ */
+static unsigned int next_csi2_port_index(struct acpi_device_software_nodes *ads,
+ unsigned int port_nr)
+{
+ unsigned int i;
+
+ for (i = 0; i < ads->num_ports; i++) {
+ struct acpi_device_software_node_port *port = &ads->ports[i];
+
+ if (port->port_nr == port_nr)
+ return i;
+
+ if (port->port_nr == NO_CSI2_PORT) {
+ port->port_nr = port_nr;
+ return i;
+ }
+ }
+
+ return NO_CSI2_PORT;
+}
+
+/* Print graph port name into a buffer, return non-zero if failed. */
+#define GRAPH_PORT_NAME(var, num) \
+ (snprintf((var), sizeof(var), SWNODE_GRAPH_PORT_NAME_FMT, (num)) >= \
+ sizeof(var))
+
static int crs_handle_cmp(const void *__a, const void *__b)
{
const struct acpi_handle_ref *a = __a, *b = __b;
@@ -258,6 +302,9 @@ static void acpi_crs_csi2_alloc_fill_swnodes(size_t ports_count, acpi_handle han
ports_count);
}
+#define ACPI_CRS_CSI2_PHY_TYPE_C 0
+#define ACPI_CRS_CSI2_PHY_TYPE_D 1
+
/**
* acpi_bus_scan_crs_csi2 - Construct software nodes out of ACPI _CRS CSI2
* resource descriptors
@@ -274,6 +321,8 @@ static void acpi_crs_csi2_alloc_fill_swnodes(size_t ports_count, acpi_handle han
* 3. Allocate memory for swnodes each ACPI device requires later on, and
* generate a list of such allocations.
*
+ * 4. Set up properties for software nodes.
+ *
* Note that struct acpi_device may not be available yet at this time.
*
* acpi_scan_lock in scan.c must be held when calling this function.
@@ -339,5 +388,312 @@ void acpi_bus_scan_crs_csi2(struct acpi_scan_context_csi2 *ctx)
this_count = this->count;
}
+ /*
+ * Allocate and set up necessary software nodes for each device and set
+ * up properties from _CRS CSI2 descriptor.
+ */
+ list_for_each_entry(csi2, &ctx->crs_csi2_head, list) {
+ struct acpi_device_software_nodes *local_swnodes;
+ struct crs_csi2_instance *inst;
+
+ local_swnodes = crs_csi2_swnode_get(csi2->handle);
+ if (WARN_ON_ONCE(!local_swnodes))
+ continue;
+
+ list_for_each_entry(inst, &csi2->buses, list) {
+ struct acpi_device_software_nodes *remote_swnodes;
+ struct acpi_device_software_node_port *local_port;
+ struct acpi_device_software_node_port *remote_port;
+ struct software_node *local_node, *remote_node;
+ unsigned int local_index, remote_index;
+ unsigned int bus_type;
+
+ remote_swnodes = crs_csi2_swnode_get(inst->remote_handle);
+ if (WARN_ON_ONCE(!remote_swnodes))
+ continue;
+
+ local_index = next_csi2_port_index(local_swnodes, inst->csi2.local_port_instance);
+ remote_index = next_csi2_port_index(remote_swnodes, inst->csi2.resource_source.index);
+
+ if (WARN_ON_ONCE(local_index >= local_swnodes->num_ports) ||
+ WARN_ON_ONCE(remote_index >= remote_swnodes->num_ports))
+ goto out_free;
+
+ switch (inst->csi2.phy_type) {
+ case ACPI_CRS_CSI2_PHY_TYPE_C:
+ bus_type = V4L2_FWNODE_BUS_TYPE_CSI2_CPHY;
+ break;
+ case ACPI_CRS_CSI2_PHY_TYPE_D:
+ bus_type = V4L2_FWNODE_BUS_TYPE_CSI2_DPHY;
+ break;
+ default:
+ acpi_handle_info(csi2->handle,
+ "ignoring CSI-2 PHY type %u\n",
+ inst->csi2.phy_type);
+ continue;
+ }
+
+ local_port = &local_swnodes->ports[local_index];
+ local_node = &local_swnodes->nodes[ACPI_DEVICE_SWNODE_EP(local_index)];
+ local_port->remote_ep_ref[0] = SOFTWARE_NODE_REFERENCE(local_node);
+ local_port->crs_csi2_local = true;
+
+ remote_port = &remote_swnodes->ports[remote_index];
+ remote_node = &remote_swnodes->nodes[ACPI_DEVICE_SWNODE_EP(remote_index)];
+ remote_port->remote_ep_ref[0] = SOFTWARE_NODE_REFERENCE(remote_node);
+
+ local_port->ep_props[ACPI_DEVICE_SWNODE_EP_REMOTE_EP] =
+ PROPERTY_ENTRY_REF_ARRAY("remote-endpoint",
+ remote_port->remote_ep_ref);
+ local_port->ep_props[ACPI_DEVICE_SWNODE_EP_BUS_TYPE] =
+ PROPERTY_ENTRY_U32("bus-type", bus_type);
+ local_port->ep_props[ACPI_DEVICE_SWNODE_EP_REG] =
+ PROPERTY_ENTRY_U32("reg", 0);
+ local_port->port_props[ACPI_DEVICE_SWNODE_PRT_REG] =
+ PROPERTY_ENTRY_U32("reg", inst->csi2.local_port_instance);
+ if (GRAPH_PORT_NAME(local_port->port_name,
+ inst->csi2.local_port_instance))
+ acpi_handle_warn(csi2->handle,
+ "name for local port %u too long",
+ inst->csi2.local_port_instance);
+
+ remote_port->ep_props[ACPI_DEVICE_SWNODE_EP_REMOTE_EP] =
+ PROPERTY_ENTRY_REF_ARRAY("remote-endpoint", local_port->remote_ep_ref);
+ remote_port->ep_props[ACPI_DEVICE_SWNODE_EP_BUS_TYPE] =
+ PROPERTY_ENTRY_U32("bus-type", bus_type);
+ remote_port->ep_props[ACPI_DEVICE_SWNODE_EP_REG] =
+ PROPERTY_ENTRY_U32("reg", 0);
+ remote_port->port_props[ACPI_DEVICE_SWNODE_PRT_REG] =
+ PROPERTY_ENTRY_U32("reg",
+ inst->csi2.resource_source.index);
+ if (GRAPH_PORT_NAME(remote_port->port_name,
+ inst->csi2.resource_source.index))
+ acpi_handle_warn(csi2->handle,
+ "name for remote port %u too long",
+ inst->csi2.resource_source.index);
+ }
+ }
+
+out_free:
kfree(handle_refs);
}
+
+/*
+ * Get the index of the next property in the property array, with a given
+ * maximum values.
+ */
+#define NEXT_PROPERTY(index, max) \
+ (WARN_ON((index) > ACPI_DEVICE_SWNODE_##max) ? \
+ ACPI_DEVICE_SWNODE_##max : (index)++)
+
+static struct fwnode_handle *get_mipi_port_handle(struct acpi_device *device,
+ unsigned int port)
+{
+ char mipi_port_name[sizeof(MIPI_IMG_PORT_PREFIX) + 2];
+
+ if (snprintf(mipi_port_name, sizeof(mipi_port_name), "%s%u",
+ MIPI_IMG_PORT_PREFIX, port) >= sizeof(mipi_port_name)) {
+ acpi_handle_info(acpi_device_handle(device),
+ "mipi port name too long for port %u\n", port);
+ return NULL;
+ }
+
+ return fwnode_get_named_child_node(acpi_fwnode_handle(device),
+ mipi_port_name);
+}
+
+static void init_port_csi2_common(struct acpi_device *device,
+ struct fwnode_handle *mipi_port_fwnode,
+ unsigned int *ep_prop_index,
+ unsigned int port_nr)
+{
+ unsigned int port_index = next_csi2_port_index(device->swnodes, port_nr);
+ struct acpi_device_software_nodes *ads = device->swnodes;
+ struct acpi_device_software_node_port *port = &ads->ports[port_index];
+ unsigned int num_lanes = 0;
+ u8 val[ARRAY_SIZE(port->data_lanes)];
+ int ret;
+
+ *ep_prop_index = ACPI_DEVICE_SWNODE_EP_CLOCK_LANES;
+
+ if (GRAPH_PORT_NAME(port->port_name, port_nr))
+ return;
+
+ ads->nodes[ACPI_DEVICE_SWNODE_PRT(port_index)] =
+ SOFTWARE_NODE(port->port_name, port->port_props,
+ &ads->nodes[ACPI_DEVICE_SWNODE_ROOT]);
+
+ ret = fwnode_property_read_u8(mipi_port_fwnode, "mipi-img-clock-lane", val);
+ if (!ret) {
+ port->ep_props[NEXT_PROPERTY(*ep_prop_index, EP_CLOCK_LANES)] =
+ PROPERTY_ENTRY_U32("clock-lanes", *val);
+ }
+ ret = fwnode_property_count_u8(mipi_port_fwnode, "mipi-img-data-lanes");
+ if (ret > 0) {
+ num_lanes = ret;
+
+ if (num_lanes > ARRAY_SIZE(port->data_lanes)) {
+ acpi_handle_warn(acpi_device_handle(device),
+ "too many data lanes (%u)\n",
+ num_lanes);
+ num_lanes = ARRAY_SIZE(port->data_lanes);
+ }
+
+ ret = fwnode_property_read_u8_array(mipi_port_fwnode, "mipi-img-data-lanes",
+ val, num_lanes);
+ if (!ret) {
+ unsigned int i;
+
+ for (i = 0; i < num_lanes; i++)
+ port->data_lanes[i] = val[i];
+
+ port->ep_props[NEXT_PROPERTY(*ep_prop_index, EP_DATA_LANES)] =
+ PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes", port->data_lanes,
+ num_lanes);
+ }
+ }
+
+ ret = fwnode_property_count_u8(mipi_port_fwnode, "mipi-img-lane-polarities");
+ if (ret > 0) {
+ unsigned int bytes = min_t(unsigned int, ret, sizeof(val));
+
+ fwnode_property_read_u8_array(mipi_port_fwnode,
+ "mipi-img-lane-polarities",
+ val, bytes);
+
+ /* Total number of lanes here is clock lane + data lanes */
+ if (bytes * BITS_PER_TYPE(u8) >= 1 + num_lanes) {
+ unsigned int i;
+
+ /* Move polarity bits to the lane polarity u32 array */
+ for (i = 0; i < 1 + num_lanes; i++)
+ port->lane_polarities[i] =
+ (bool)(val[i >> 3] & (1 << (i & 7)));
+
+ port->ep_props[NEXT_PROPERTY(*ep_prop_index, EP_LANE_POLARITIES)] =
+ PROPERTY_ENTRY_U32_ARRAY_LEN("lane-polarities",
+ port->lane_polarities,
+ 1 + num_lanes);
+ } else {
+ acpi_handle_warn(acpi_device_handle(device),
+ "too few lane polarity bytes (%u)\n",
+ bytes);
+ }
+ }
+
+ ads->nodes[ACPI_DEVICE_SWNODE_EP(port_index)] =
+ SOFTWARE_NODE("endpoint@0", ads->ports[port_index].ep_props,
+ &ads->nodes[ACPI_DEVICE_SWNODE_PRT(port_index)]);
+}
+
+static void init_port_csi2_local(struct acpi_device *device,
+ unsigned int port_nr)
+{
+ unsigned int port_index = next_csi2_port_index(device->swnodes, port_nr);
+ struct fwnode_handle *mipi_port_fwnode =
+ get_mipi_port_handle(device, port_nr);
+ struct acpi_device_software_node_port *port =
+ &device->swnodes->ports[port_index];
+ unsigned int ep_prop_index;
+ int ret;
+
+ init_port_csi2_common(device, mipi_port_fwnode, &ep_prop_index, port_nr);
+
+ ret = fwnode_property_count_u64(mipi_port_fwnode, "mipi-img-link-frequencies");
+ if (ret > 0) {
+ unsigned int num_link_freqs = ret;
+
+ if (num_link_freqs > ARRAY_SIZE(port->link_frequencies)) {
+ acpi_handle_info(acpi_device_handle(device),
+ "too many link frequencies %u\n",
+ num_link_freqs);
+ num_link_freqs = ARRAY_SIZE(port->link_frequencies);
+ }
+
+ ret = fwnode_property_read_u64_array(mipi_port_fwnode,
+ "mipi-img-link-frequencies",
+ port->link_frequencies,
+ num_link_freqs);
+ if (!ret)
+ port->ep_props[NEXT_PROPERTY(ep_prop_index, EP_LINK_FREQUENCIES)] =
+ PROPERTY_ENTRY_U64_ARRAY_LEN("link-frequencies",
+ port->link_frequencies,
+ num_link_freqs);
+ else
+ acpi_handle_info(acpi_device_handle(device),
+ "can't get link frequencies (%d)\n",
+ ret);
+ }
+
+ fwnode_handle_put(mipi_port_fwnode);
+}
+
+static void init_port_csi2_remote(struct acpi_device *device,
+ unsigned int port_nr)
+{
+ struct fwnode_handle *mipi_port_fwnode = get_mipi_port_handle(device, port_nr);
+ unsigned int ep_prop_index;
+
+ init_port_csi2_common(device, mipi_port_fwnode, &ep_prop_index, port_nr);
+
+ fwnode_handle_put(mipi_port_fwnode);
+}
+
+/**
+ * acpi_init_swnodes - Set up software nodes for properties gathered elsewhere
+ *
+ * @device: ACPI device for which the software nodes are initialised
+ *
+ * Initialise and register software nodes for properties for which the data is
+ * gathered elsewhere, e.g. _CRS CSI-2 descriptors. The process itself takes
+ * place before this function is called.
+ *
+ * acpi_scan_lock in scan.c must be held when calling this function.
+ */
+void acpi_init_swnodes(struct acpi_device *device)
+{
+ struct acpi_device_software_nodes *ads;
+ struct acpi_buffer buffer = { .length = ACPI_ALLOCATE_BUFFER };
+ acpi_handle handle = acpi_device_handle(device);
+ struct fwnode_handle *primary;
+ acpi_status status;
+ unsigned int i;
+ int ret;
+
+ device->swnodes = ads = crs_csi2_swnode_get(handle);
+ if (!ads)
+ return;
+
+ status = acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
+ if (ACPI_FAILURE(status)) {
+ acpi_handle_warn(handle, "cannot get path name\n");
+ return;
+ }
+
+ ads->nodes[ACPI_DEVICE_SWNODE_ROOT] =
+ SOFTWARE_NODE(buffer.pointer, ads->dev_props, NULL);
+
+ for (i = 0; i < ads->num_ports; i++) {
+ struct acpi_device_software_node_port *port = &ads->ports[i];
+
+ if (port->crs_csi2_local)
+ init_port_csi2_local(device, port->port_nr);
+ else
+ init_port_csi2_remote(device, port->port_nr);
+ }
+
+ ret = software_node_register_node_group(ads->nodeptrs);
+ if (ret < 0) {
+ acpi_handle_warn(handle,
+ "cannot register software nodes (%d)!\n", ret);
+ device->swnodes = NULL;
+ return;
+ }
+
+ /*
+ * Note we can't use set_secondary_fwnode() here as the device's
+ * primary fwnode hasn't been set yet.
+ */
+ primary = acpi_fwnode_handle(device);
+ primary->secondary = software_node_fwnode(ads->nodes);
+}
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index ddf7701a57d0..bf0645854cf1 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -449,10 +449,28 @@ static void acpi_free_power_resources_lists(struct acpi_device *device)
}
}
+static void acpi_free_swnodes(struct acpi_device *device)
+{
+ struct acpi_device_software_nodes *ads = device->swnodes;
+ struct fwnode_handle *primary;
+
+ if (!ads)
+ return;
+
+ software_node_unregister_node_group(ads->nodeptrs);
+ primary = acpi_fwnode_handle(device);
+ primary->secondary = NULL;
+ kfree(ads->nodes[ACPI_DEVICE_SWNODE_ROOT].name);
+ kfree(ads);
+
+ device->swnodes = NULL;
+}
+
static void acpi_device_release(struct device *dev)
{
struct acpi_device *acpi_dev = to_acpi_device(dev);
+ acpi_free_swnodes(acpi_dev);
acpi_free_properties(acpi_dev);
acpi_free_pnp_ids(&acpi_dev->pnp);
acpi_free_power_resources_lists(acpi_dev);
@@ -2050,8 +2068,7 @@ struct acpi_postponed_handle {
* Add a given ACPI handle to a list of ACPI objects for which the creation
* of the device objects is to be postponed.
*/
-static void acpi_bus_handle_postpone(acpi_handle handle,
- struct list_head *head)
+static void acpi_bus_handle_postpone(acpi_handle handle, struct list_head *head)
{
struct acpi_postponed_handle *ph;
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index e4e8faac9212..29d1e4d49d0c 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -360,15 +360,84 @@ struct acpi_device_data {
struct acpi_gpio_mapping;
+enum acpi_device_swnode_dev_props {
+ ACPI_DEVICE_SWNODE_DEV_NUM_OF,
+ ACPI_DEVICE_SWNODE_DEV_NUM_ENTRIES
+};
+
+enum acpi_device_swnode_port_props {
+ ACPI_DEVICE_SWNODE_PRT_REG,
+ ACPI_DEVICE_SWNODE_PRT_NUM_OF,
+ ACPI_DEVICE_SWNODE_PRT_NUM_ENTRIES
+};
+
+enum acpi_device_swnode_ep_props {
+ ACPI_DEVICE_SWNODE_EP_REMOTE_EP,
+ ACPI_DEVICE_SWNODE_EP_BUS_TYPE,
+ ACPI_DEVICE_SWNODE_EP_REG,
+ ACPI_DEVICE_SWNODE_EP_CLOCK_LANES,
+ ACPI_DEVICE_SWNODE_EP_DATA_LANES,
+ ACPI_DEVICE_SWNODE_EP_LANE_POLARITIES,
+ /* TX only */
+ ACPI_DEVICE_SWNODE_EP_LINK_FREQUENCIES,
+ ACPI_DEVICE_SWNODE_EP_NUM_OF,
+ ACPI_DEVICE_SWNODE_EP_NUM_ENTRIES
+};
+
+#define ACPI_DEVICE_SWNODE_ROOT 0
+/*
+ * Each device has a root swnode plus two times as many nodes as the
+ * number of CSI-2 ports.
+ */
+#define ACPI_DEVICE_SWNODE_PRT(port) (1 + 2 * (port))
+#define ACPI_DEVICE_SWNODE_EP(endpoint) \
+ (ACPI_DEVICE_SWNODE_PRT(endpoint) + 1)
+
+#define ACPI_DEVICE_SWNODE_CSI2_DATA_LANES 4
+
+/**
+ * struct acpi_device_software_node_port: Software nodes for MIPI DisCo for
+ * Imaging support
+ * @port_name: the name of the port
+ * @data_lanes: "data-lanes" property values
+ * @lane_polarities: "lane-polarities" property values
+ * @link_frequencies: "link_frequencies" property values
+ * @port_nr: the number of the port
+ * @crs_crs2_local: whether the _CRS CSI2 record is local to the port (i.e. the
+ * port is a transmitter port)
+ * port_props: the port properties
+ * ep_props: the endpoint properties
+ * remote_ep_ref: reference to the remote endpoint
+ */
struct acpi_device_software_node_port {
+ char port_name[8];
+ u32 data_lanes[ACPI_DEVICE_SWNODE_CSI2_DATA_LANES];
+ u32 lane_polarities[1 /* clock lane */ +
+ ACPI_DEVICE_SWNODE_CSI2_DATA_LANES];
+ u64 link_frequencies[4];
unsigned int port_nr;
+ bool crs_csi2_local;
+
+ struct property_entry port_props[ACPI_DEVICE_SWNODE_PRT_NUM_ENTRIES];
+ struct property_entry ep_props[ACPI_DEVICE_SWNODE_EP_NUM_ENTRIES];
+
+ struct software_node_ref_args remote_ep_ref[1];
};
+/**
+ * struct acpi_device_software_nodes - Software nodes for an ACPI device
+ * @ports: information related to each port and endpoint within a port
+ * @nodes: software nodes for root as well as ports and endpoints
+ * @nodeprts: array of software node pointers, for (un)registering them
+ * @num_ports: the number of ports
+ */
struct acpi_device_software_nodes {
struct acpi_device_software_node_port *ports;
struct software_node *nodes;
const struct software_node **nodeptrs;
unsigned int num_ports;
+
+ struct property_entry dev_props[ACPI_DEVICE_SWNODE_DEV_NUM_ENTRIES];
};
/* Device */
@@ -377,6 +446,7 @@ struct acpi_device {
int device_type;
acpi_handle handle; /* no handle for fixed hardware */
struct fwnode_handle fwnode;
+ struct acpi_device_software_nodes *swnodes;
struct list_head wakeup_list;
struct list_head del_list;
struct acpi_device_status status;
--
2.30.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v8 05/10] ACPI: property: Prepare generating swnodes for ACPI and DisCo for Imaging
2023-03-29 10:09 ` [PATCH v8 05/10] ACPI: property: Prepare generating swnodes for ACPI and DisCo for Imaging Sakari Ailus
@ 2023-03-29 14:39 ` Andy Shevchenko
2023-05-17 10:53 ` Rafael J. Wysocki
1 sibling, 0 replies; 36+ messages in thread
From: Andy Shevchenko @ 2023-03-29 14:39 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-acpi, linux-media, rafael, heikki.krogerus
On Wed, Mar 29, 2023 at 01:09:46PM +0300, Sakari Ailus wrote:
> Prepare generating software nodes for information parsed from ACPI _CRS for
> CSI-2 as well as MIPI DisCo for Imaging spec. The software nodes are
> compliant with existing ACPI or DT definitions and are parsed by relevant
> drivers without changes.
There is discussion is ongoing in the previous version of the series for this
patch.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v8 05/10] ACPI: property: Prepare generating swnodes for ACPI and DisCo for Imaging
2023-03-29 10:09 ` [PATCH v8 05/10] ACPI: property: Prepare generating swnodes for ACPI and DisCo for Imaging Sakari Ailus
2023-03-29 14:39 ` Andy Shevchenko
@ 2023-05-17 10:53 ` Rafael J. Wysocki
2023-05-17 11:45 ` Sakari Ailus
1 sibling, 1 reply; 36+ messages in thread
From: Rafael J. Wysocki @ 2023-05-17 10:53 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-acpi, linux-media, rafael, andriy.shevchenko,
heikki.krogerus
On Wed, Mar 29, 2023 at 12:10 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Prepare generating software nodes for information parsed from ACPI _CRS for
> CSI-2 as well as MIPI DisCo for Imaging spec. The software nodes are
> compliant with existing ACPI or DT definitions and are parsed by relevant
> drivers without changes.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/acpi/internal.h | 3 +
> drivers/acpi/mipi.c | 358 +++++++++++++++++++++++++++++++++++++++-
> drivers/acpi/scan.c | 21 ++-
> include/acpi/acpi_bus.h | 70 ++++++++
> 4 files changed, 449 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index aa5f9c69dbbe..1634a177c75e 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -304,9 +304,12 @@ static inline void acpi_init_lpit(void) { }
> ACPI _CRS CSI2 and MIPI DisCo for Imaging conversion
> -------------------------------------------------------------------------- */
>
> +#define MIPI_IMG_PORT_PREFIX "mipi-img-port-"
> +
> void acpi_crs_csi2_swnodes_del_free(void);
> void acpi_bus_scan_check_crs_csi2(acpi_handle handle, struct acpi_scan_context *ctx);
> void acpi_bus_scan_crs_csi2_release(struct list_head *crs_csi2_handles);
> void acpi_bus_scan_crs_csi2(struct acpi_scan_context_csi2 *ctx);
> +void acpi_init_swnodes(struct acpi_device *device);
>
> #endif /* _ACPI_INTERNAL_H_ */
> diff --git a/drivers/acpi/mipi.c b/drivers/acpi/mipi.c
> index d719c879eb83..03079f78bd2c 100644
> --- a/drivers/acpi/mipi.c
> +++ b/drivers/acpi/mipi.c
> @@ -3,7 +3,8 @@
> * MIPI DisCo for Imaging support.
> *
> * Support MIPI DisCo for Imaging by parsing ACPI _CRS CSI2 records and DisCo
> - * for Imaging data structures.
> + * for Imaging data structures and generating nodes and properties using
> + * software nodes compliant with DT definitions of the similar scope.
> *
> * Also see <URL:https://www.mipi.org/specifications/mipi-disco-imaging>.
> *
> @@ -20,6 +21,8 @@
> #include <linux/sort.h>
> #include <linux/string.h>
>
> +#include <media/v4l2-fwnode.h>
> +
> #include "internal.h"
>
> /* Temporary ACPI device handle to software node data structure mapping */
> @@ -31,6 +34,18 @@ struct crs_csi2_swnodes {
>
> static LIST_HEAD(crs_csi2_swnodes);
>
> +/* Obtain pre-allocated software nodes for an ACPI device handle */
> +static struct acpi_device_software_nodes *crs_csi2_swnode_get(acpi_handle handle)
> +{
> + struct crs_csi2_swnodes *swnodes;
> +
> + list_for_each_entry(swnodes, &crs_csi2_swnodes, list)
> + if (swnodes->handle == handle)
> + return swnodes->ads;
> +
> + return NULL;
> +}
> +
> static void crs_csi2_swnode_del_free(struct crs_csi2_swnodes *swnodes)
> {
> list_del(&swnodes->list);
> @@ -166,6 +181,35 @@ struct acpi_handle_ref {
>
> #define NO_CSI2_PORT (UINT_MAX - 1)
>
> +/*
> + * Return next free entry in ports array of a software nodes related to an ACPI
> + * device.
> + */
> +static unsigned int next_csi2_port_index(struct acpi_device_software_nodes *ads,
> + unsigned int port_nr)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < ads->num_ports; i++) {
> + struct acpi_device_software_node_port *port = &ads->ports[i];
> +
> + if (port->port_nr == port_nr)
> + return i;
> +
> + if (port->port_nr == NO_CSI2_PORT) {
> + port->port_nr = port_nr;
> + return i;
> + }
> + }
> +
> + return NO_CSI2_PORT;
> +}
> +
> +/* Print graph port name into a buffer, return non-zero if failed. */
> +#define GRAPH_PORT_NAME(var, num) \
> + (snprintf((var), sizeof(var), SWNODE_GRAPH_PORT_NAME_FMT, (num)) >= \
> + sizeof(var))
> +
> static int crs_handle_cmp(const void *__a, const void *__b)
> {
> const struct acpi_handle_ref *a = __a, *b = __b;
> @@ -258,6 +302,9 @@ static void acpi_crs_csi2_alloc_fill_swnodes(size_t ports_count, acpi_handle han
> ports_count);
> }
>
> +#define ACPI_CRS_CSI2_PHY_TYPE_C 0
> +#define ACPI_CRS_CSI2_PHY_TYPE_D 1
> +
> /**
> * acpi_bus_scan_crs_csi2 - Construct software nodes out of ACPI _CRS CSI2
> * resource descriptors
> @@ -274,6 +321,8 @@ static void acpi_crs_csi2_alloc_fill_swnodes(size_t ports_count, acpi_handle han
> * 3. Allocate memory for swnodes each ACPI device requires later on, and
> * generate a list of such allocations.
> *
> + * 4. Set up properties for software nodes.
> + *
> * Note that struct acpi_device may not be available yet at this time.
> *
> * acpi_scan_lock in scan.c must be held when calling this function.
> @@ -339,5 +388,312 @@ void acpi_bus_scan_crs_csi2(struct acpi_scan_context_csi2 *ctx)
> this_count = this->count;
> }
>
> + /*
> + * Allocate and set up necessary software nodes for each device and set
> + * up properties from _CRS CSI2 descriptor.
> + */
> + list_for_each_entry(csi2, &ctx->crs_csi2_head, list) {
> + struct acpi_device_software_nodes *local_swnodes;
> + struct crs_csi2_instance *inst;
> +
> + local_swnodes = crs_csi2_swnode_get(csi2->handle);
> + if (WARN_ON_ONCE(!local_swnodes))
> + continue;
> +
> + list_for_each_entry(inst, &csi2->buses, list) {
> + struct acpi_device_software_nodes *remote_swnodes;
> + struct acpi_device_software_node_port *local_port;
> + struct acpi_device_software_node_port *remote_port;
> + struct software_node *local_node, *remote_node;
> + unsigned int local_index, remote_index;
> + unsigned int bus_type;
> +
> + remote_swnodes = crs_csi2_swnode_get(inst->remote_handle);
> + if (WARN_ON_ONCE(!remote_swnodes))
> + continue;
> +
> + local_index = next_csi2_port_index(local_swnodes, inst->csi2.local_port_instance);
> + remote_index = next_csi2_port_index(remote_swnodes, inst->csi2.resource_source.index);
> +
> + if (WARN_ON_ONCE(local_index >= local_swnodes->num_ports) ||
> + WARN_ON_ONCE(remote_index >= remote_swnodes->num_ports))
> + goto out_free;
> +
> + switch (inst->csi2.phy_type) {
> + case ACPI_CRS_CSI2_PHY_TYPE_C:
> + bus_type = V4L2_FWNODE_BUS_TYPE_CSI2_CPHY;
> + break;
> + case ACPI_CRS_CSI2_PHY_TYPE_D:
> + bus_type = V4L2_FWNODE_BUS_TYPE_CSI2_DPHY;
> + break;
> + default:
> + acpi_handle_info(csi2->handle,
> + "ignoring CSI-2 PHY type %u\n",
> + inst->csi2.phy_type);
> + continue;
> + }
> +
> + local_port = &local_swnodes->ports[local_index];
> + local_node = &local_swnodes->nodes[ACPI_DEVICE_SWNODE_EP(local_index)];
> + local_port->remote_ep_ref[0] = SOFTWARE_NODE_REFERENCE(local_node);
This looks odd. Is local_port pointing to its own node as a remote
endpont, or am I confused?
> + local_port->crs_csi2_local = true;
> +
> + remote_port = &remote_swnodes->ports[remote_index];
> + remote_node = &remote_swnodes->nodes[ACPI_DEVICE_SWNODE_EP(remote_index)];
> + remote_port->remote_ep_ref[0] = SOFTWARE_NODE_REFERENCE(remote_node);
Analogously here.
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v8 05/10] ACPI: property: Prepare generating swnodes for ACPI and DisCo for Imaging
2023-05-17 10:53 ` Rafael J. Wysocki
@ 2023-05-17 11:45 ` Sakari Ailus
2023-05-17 12:22 ` Rafael J. Wysocki
0 siblings, 1 reply; 36+ messages in thread
From: Sakari Ailus @ 2023-05-17 11:45 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: linux-acpi, linux-media, andriy.shevchenko, heikki.krogerus
Hi Rafael,
Thanks for the review.
On Wed, May 17, 2023 at 12:53:43PM +0200, Rafael J. Wysocki wrote:
> > + list_for_each_entry(csi2, &ctx->crs_csi2_head, list) {
> > + struct acpi_device_software_nodes *local_swnodes;
> > + struct crs_csi2_instance *inst;
> > +
> > + local_swnodes = crs_csi2_swnode_get(csi2->handle);
> > + if (WARN_ON_ONCE(!local_swnodes))
> > + continue;
> > +
> > + list_for_each_entry(inst, &csi2->buses, list) {
> > + struct acpi_device_software_nodes *remote_swnodes;
> > + struct acpi_device_software_node_port *local_port;
> > + struct acpi_device_software_node_port *remote_port;
> > + struct software_node *local_node, *remote_node;
> > + unsigned int local_index, remote_index;
> > + unsigned int bus_type;
> > +
> > + remote_swnodes = crs_csi2_swnode_get(inst->remote_handle);
> > + if (WARN_ON_ONCE(!remote_swnodes))
> > + continue;
> > +
> > + local_index = next_csi2_port_index(local_swnodes, inst->csi2.local_port_instance);
> > + remote_index = next_csi2_port_index(remote_swnodes, inst->csi2.resource_source.index);
> > +
> > + if (WARN_ON_ONCE(local_index >= local_swnodes->num_ports) ||
> > + WARN_ON_ONCE(remote_index >= remote_swnodes->num_ports))
> > + goto out_free;
> > +
> > + switch (inst->csi2.phy_type) {
> > + case ACPI_CRS_CSI2_PHY_TYPE_C:
> > + bus_type = V4L2_FWNODE_BUS_TYPE_CSI2_CPHY;
> > + break;
> > + case ACPI_CRS_CSI2_PHY_TYPE_D:
> > + bus_type = V4L2_FWNODE_BUS_TYPE_CSI2_DPHY;
> > + break;
> > + default:
> > + acpi_handle_info(csi2->handle,
> > + "ignoring CSI-2 PHY type %u\n",
> > + inst->csi2.phy_type);
> > + continue;
> > + }
> > +
> > + local_port = &local_swnodes->ports[local_index];
> > + local_node = &local_swnodes->nodes[ACPI_DEVICE_SWNODE_EP(local_index)];
> > + local_port->remote_ep_ref[0] = SOFTWARE_NODE_REFERENCE(local_node);
>
> This looks odd. Is local_port pointing to its own node as a remote
> endpont, or am I confused?
This is a reference to a software node that will be, in turn, referenced by
the "remote-endpoint" property entry in the remote node. Look for
ACPI_DEVICE_SWNODE_EP_REMOTE_EP a few lines below these.
>
> > + local_port->crs_csi2_local = true;
> > +
> > + remote_port = &remote_swnodes->ports[remote_index];
> > + remote_node = &remote_swnodes->nodes[ACPI_DEVICE_SWNODE_EP(remote_index)];
> > + remote_port->remote_ep_ref[0] = SOFTWARE_NODE_REFERENCE(remote_node);
>
> Analogously here.
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v8 05/10] ACPI: property: Prepare generating swnodes for ACPI and DisCo for Imaging
2023-05-17 11:45 ` Sakari Ailus
@ 2023-05-17 12:22 ` Rafael J. Wysocki
2023-05-17 13:50 ` Rafael J. Wysocki
0 siblings, 1 reply; 36+ messages in thread
From: Rafael J. Wysocki @ 2023-05-17 12:22 UTC (permalink / raw)
To: Sakari Ailus
Cc: Rafael J. Wysocki, linux-acpi, linux-media, andriy.shevchenko,
heikki.krogerus
On Wed, May 17, 2023 at 1:54 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Rafael,
>
> Thanks for the review.
>
> On Wed, May 17, 2023 at 12:53:43PM +0200, Rafael J. Wysocki wrote:
> > > + list_for_each_entry(csi2, &ctx->crs_csi2_head, list) {
> > > + struct acpi_device_software_nodes *local_swnodes;
> > > + struct crs_csi2_instance *inst;
> > > +
> > > + local_swnodes = crs_csi2_swnode_get(csi2->handle);
> > > + if (WARN_ON_ONCE(!local_swnodes))
> > > + continue;
> > > +
> > > + list_for_each_entry(inst, &csi2->buses, list) {
> > > + struct acpi_device_software_nodes *remote_swnodes;
> > > + struct acpi_device_software_node_port *local_port;
> > > + struct acpi_device_software_node_port *remote_port;
> > > + struct software_node *local_node, *remote_node;
> > > + unsigned int local_index, remote_index;
> > > + unsigned int bus_type;
> > > +
> > > + remote_swnodes = crs_csi2_swnode_get(inst->remote_handle);
> > > + if (WARN_ON_ONCE(!remote_swnodes))
> > > + continue;
> > > +
> > > + local_index = next_csi2_port_index(local_swnodes, inst->csi2.local_port_instance);
> > > + remote_index = next_csi2_port_index(remote_swnodes, inst->csi2.resource_source.index);
> > > +
> > > + if (WARN_ON_ONCE(local_index >= local_swnodes->num_ports) ||
> > > + WARN_ON_ONCE(remote_index >= remote_swnodes->num_ports))
> > > + goto out_free;
> > > +
> > > + switch (inst->csi2.phy_type) {
> > > + case ACPI_CRS_CSI2_PHY_TYPE_C:
> > > + bus_type = V4L2_FWNODE_BUS_TYPE_CSI2_CPHY;
> > > + break;
> > > + case ACPI_CRS_CSI2_PHY_TYPE_D:
> > > + bus_type = V4L2_FWNODE_BUS_TYPE_CSI2_DPHY;
> > > + break;
> > > + default:
> > > + acpi_handle_info(csi2->handle,
> > > + "ignoring CSI-2 PHY type %u\n",
> > > + inst->csi2.phy_type);
> > > + continue;
> > > + }
> > > +
> > > + local_port = &local_swnodes->ports[local_index];
> > > + local_node = &local_swnodes->nodes[ACPI_DEVICE_SWNODE_EP(local_index)];
> > > + local_port->remote_ep_ref[0] = SOFTWARE_NODE_REFERENCE(local_node);
> >
> > This looks odd. Is local_port pointing to its own node as a remote
> > endpont, or am I confused?
>
> This is a reference to a software node that will be, in turn, referenced by
> the "remote-endpoint" property entry in the remote node. Look for
> ACPI_DEVICE_SWNODE_EP_REMOTE_EP a few lines below these.
To be precise, IIUC, it is going to be the "remote-endpoint" value for
the remote node.
OK, thanks for the explanation. This isn't exactly straightforward TBH.
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v8 05/10] ACPI: property: Prepare generating swnodes for ACPI and DisCo for Imaging
2023-05-17 12:22 ` Rafael J. Wysocki
@ 2023-05-17 13:50 ` Rafael J. Wysocki
2023-05-17 17:34 ` Sakari Ailus
0 siblings, 1 reply; 36+ messages in thread
From: Rafael J. Wysocki @ 2023-05-17 13:50 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-acpi, linux-media, andriy.shevchenko, heikki.krogerus
On Wed, May 17, 2023 at 2:22 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, May 17, 2023 at 1:54 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
[cut]
> > > > + local_port = &local_swnodes->ports[local_index];
> > > > + local_node = &local_swnodes->nodes[ACPI_DEVICE_SWNODE_EP(local_index)];
> > > > + local_port->remote_ep_ref[0] = SOFTWARE_NODE_REFERENCE(local_node);
> > >
> > > This looks odd. Is local_port pointing to its own node as a remote
> > > endpont, or am I confused?
> >
> > This is a reference to a software node that will be, in turn, referenced by
> > the "remote-endpoint" property entry in the remote node. Look for
> > ACPI_DEVICE_SWNODE_EP_REMOTE_EP a few lines below these.
>
> To be precise, IIUC, it is going to be the "remote-endpoint" value for
> the remote node.
>
> OK, thanks for the explanation. This isn't exactly straightforward TBH.
So I'd arrange it so that the value of the "remote-endpoint" property
in a given node is stored in that node (the value itself being a
reference to another endpoint).
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v8 05/10] ACPI: property: Prepare generating swnodes for ACPI and DisCo for Imaging
2023-05-17 13:50 ` Rafael J. Wysocki
@ 2023-05-17 17:34 ` Sakari Ailus
0 siblings, 0 replies; 36+ messages in thread
From: Sakari Ailus @ 2023-05-17 17:34 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: linux-acpi, linux-media, andriy.shevchenko, heikki.krogerus
On Wed, May 17, 2023 at 03:50:47PM +0200, Rafael J. Wysocki wrote:
> On Wed, May 17, 2023 at 2:22 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Wed, May 17, 2023 at 1:54 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
>
> [cut]
>
> > > > > + local_port = &local_swnodes->ports[local_index];
> > > > > + local_node = &local_swnodes->nodes[ACPI_DEVICE_SWNODE_EP(local_index)];
> > > > > + local_port->remote_ep_ref[0] = SOFTWARE_NODE_REFERENCE(local_node);
> > > >
> > > > This looks odd. Is local_port pointing to its own node as a remote
> > > > endpont, or am I confused?
> > >
> > > This is a reference to a software node that will be, in turn, referenced by
> > > the "remote-endpoint" property entry in the remote node. Look for
> > > ACPI_DEVICE_SWNODE_EP_REMOTE_EP a few lines below these.
> >
> > To be precise, IIUC, it is going to be the "remote-endpoint" value for
> > the remote node.
> >
> > OK, thanks for the explanation. This isn't exactly straightforward TBH.
>
> So I'd arrange it so that the value of the "remote-endpoint" property
> in a given node is stored in that node (the value itself being a
> reference to another endpoint).
Fine for me.
--
Sakari Ailus
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v8 06/10] ACPI: scan: Generate software nodes based on MIPI DisCo for Imaging
2023-03-29 10:09 [PATCH v8 00/10] ACPI _CRS CSI-2 and MIPI DisCo for Imaging support Sakari Ailus
` (4 preceding siblings ...)
2023-03-29 10:09 ` [PATCH v8 05/10] ACPI: property: Prepare generating swnodes for ACPI and DisCo for Imaging Sakari Ailus
@ 2023-03-29 10:09 ` Sakari Ailus
2023-03-29 10:09 ` [PATCH v8 07/10] ACPI: property: Dig "rotation" property for devices with CSI2 _CRS Sakari Ailus
` (3 subsequent siblings)
9 siblings, 0 replies; 36+ messages in thread
From: Sakari Ailus @ 2023-03-29 10:09 UTC (permalink / raw)
To: linux-acpi; +Cc: linux-media, rafael, andriy.shevchenko, heikki.krogerus
Generate software nodes for driver use, based on MIPI DisCo for Imaging
definitions.
During the (sub-)namespace walk, ACPI device nodes are created but the
drivers aren't probed for the devices yet. A convenient way to determine
which ACPI devices this applies to is to find a hierarchical data node that
begins with "mipi-img-port-". These devices need software nodes that need
to be present before probing, and can only be constructed once the related
_CRS CSI2 records have been parsed.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/acpi/internal.h | 11 ++++--
drivers/acpi/power.c | 2 +-
drivers/acpi/property.c | 48 +++++++++++++++++--------
drivers/acpi/scan.c | 80 +++++++++++++++++++++++++++++++++--------
4 files changed, 109 insertions(+), 32 deletions(-)
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 1634a177c75e..36799cde281c 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -111,17 +111,23 @@ struct acpi_scan_context_csi2 {
/**
* struct acpi_scan_context - Context for scanning ACPI devices
* @postponed_head: The list head of the postponed ACPI handles
+ * @mipi_img_head: The list of ACPI devices with MIPI DisCo for Imaging related
+ * properties
* @device: The first encountered device, typically the root of the scanned tree
* @csi2: Context for scanning _CRS CSI2 resource descriptors
*/
struct acpi_scan_context {
struct list_head postponed_head;
+ struct list_head mipi_img_head;
struct acpi_device *device;
struct acpi_scan_context_csi2 csi2;
};
+void acpi_bus_device_postpone(struct acpi_device *device,
+ struct list_head *head);
void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
- int type, void (*release)(struct device *));
+ int type, void (*release)(struct device *),
+ struct list_head *mipi_img_head);
int acpi_tie_acpi_dev(struct acpi_device *adev);
int acpi_device_add(struct acpi_device *device);
int acpi_device_setup_files(struct acpi_device *dev);
@@ -275,7 +281,8 @@ static inline bool force_storage_d3(void)
-------------------------------------------------------------------------- */
#define ACPI_DT_NAMESPACE_HID "PRP0001"
-void acpi_init_properties(struct acpi_device *adev);
+void acpi_init_properties(struct acpi_device *adev,
+ struct list_head *mipi_img_head);
void acpi_free_properties(struct acpi_device *adev);
#ifdef CONFIG_X86
diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c
index 23507d29f000..1096cdbf3d28 100644
--- a/drivers/acpi/power.c
+++ b/drivers/acpi/power.c
@@ -945,7 +945,7 @@ struct acpi_device *acpi_add_power_resource(acpi_handle handle)
device = &resource->device;
acpi_init_device_object(device, handle, ACPI_BUS_TYPE_POWER,
- acpi_release_power_resource);
+ acpi_release_power_resource, NULL);
mutex_init(&resource->resource_lock);
INIT_LIST_HEAD(&resource->list_node);
INIT_LIST_HEAD(&resource->dependents);
diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 08831ffba26c..1892787e73a6 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -62,10 +62,12 @@ static const guid_t buffer_prop_guid =
GUID_INIT(0xedb12dd0, 0x363d, 0x4085,
0xa3, 0xd2, 0x49, 0x52, 0x2c, 0xa1, 0x60, 0xc4);
-static bool acpi_enumerate_nondev_subnodes(acpi_handle scope,
+static bool acpi_enumerate_nondev_subnodes(struct acpi_device *device,
+ acpi_handle scope,
union acpi_object *desc,
struct acpi_device_data *data,
- struct fwnode_handle *parent);
+ struct fwnode_handle *parent,
+ struct list_head *mipi_img_head);
static bool acpi_extract_properties(acpi_handle handle,
union acpi_object *desc,
struct acpi_device_data *data);
@@ -103,11 +105,12 @@ static bool acpi_nondev_subnode_extract(union acpi_object *desc,
*/
status = acpi_get_parent(handle, &scope);
if (ACPI_SUCCESS(status)
- && acpi_enumerate_nondev_subnodes(scope, desc, &dn->data,
- &dn->fwnode))
+ && acpi_enumerate_nondev_subnodes(NULL, scope, desc,
+ &dn->data, &dn->fwnode,
+ NULL))
result = true;
- } else if (acpi_enumerate_nondev_subnodes(NULL, desc, &dn->data,
- &dn->fwnode)) {
+ } else if (acpi_enumerate_nondev_subnodes(NULL, NULL, desc, &dn->data,
+ &dn->fwnode, NULL)) {
result = true;
}
@@ -163,11 +166,14 @@ static bool acpi_nondev_subnode_ok(acpi_handle scope,
return acpi_nondev_subnode_data_ok(handle, link, list, parent);
}
-static bool acpi_add_nondev_subnodes(acpi_handle scope,
+static bool acpi_add_nondev_subnodes(struct acpi_device *device,
+ acpi_handle scope,
union acpi_object *links,
struct list_head *list,
- struct fwnode_handle *parent)
+ struct fwnode_handle *parent,
+ struct list_head *mipi_img_head)
{
+ bool has_mipi_img_nodes = false;
bool ret = false;
int i;
@@ -188,6 +194,13 @@ static bool acpi_add_nondev_subnodes(acpi_handle scope,
/* The second one may be a string, a reference or a package. */
switch (link->package.elements[1].type) {
case ACPI_TYPE_STRING:
+ if (!has_mipi_img_nodes && mipi_img_head &&
+ !strncmp(MIPI_IMG_PORT_PREFIX,
+ link->package.elements[0].string.pointer,
+ strlen(MIPI_IMG_PORT_PREFIX))) {
+ acpi_bus_device_postpone(device, mipi_img_head);
+ has_mipi_img_nodes = true;
+ }
result = acpi_nondev_subnode_ok(scope, link, list,
parent);
break;
@@ -211,10 +224,12 @@ static bool acpi_add_nondev_subnodes(acpi_handle scope,
return ret;
}
-static bool acpi_enumerate_nondev_subnodes(acpi_handle scope,
+static bool acpi_enumerate_nondev_subnodes(struct acpi_device *device,
+ acpi_handle scope,
union acpi_object *desc,
struct acpi_device_data *data,
- struct fwnode_handle *parent)
+ struct fwnode_handle *parent,
+ struct list_head *mipi_img_head)
{
int i;
@@ -238,8 +253,9 @@ static bool acpi_enumerate_nondev_subnodes(acpi_handle scope,
if (!guid_equal((guid_t *)guid->buffer.pointer, &ads_guid))
continue;
- return acpi_add_nondev_subnodes(scope, links, &data->subnodes,
- parent);
+ return acpi_add_nondev_subnodes(device, scope, links,
+ &data->subnodes, parent,
+ mipi_img_head);
}
return false;
@@ -533,7 +549,8 @@ static bool acpi_extract_properties(acpi_handle scope, union acpi_object *desc,
return !list_empty(&data->properties);
}
-void acpi_init_properties(struct acpi_device *adev)
+void acpi_init_properties(struct acpi_device *adev,
+ struct list_head *mipi_img_head)
{
struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
struct acpi_hardware_id *hwid;
@@ -567,8 +584,9 @@ void acpi_init_properties(struct acpi_device *adev)
if (acpi_of)
acpi_init_of_compatible(adev);
}
- if (acpi_enumerate_nondev_subnodes(adev->handle, buf.pointer,
- &adev->data, acpi_fwnode_handle(adev)))
+ if (acpi_enumerate_nondev_subnodes(adev, adev->handle, buf.pointer,
+ &adev->data, acpi_fwnode_handle(adev),
+ mipi_img_head))
adev->data.pointer = buf.pointer;
if (!adev->data.pointer) {
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index bf0645854cf1..a3ad0060c2fc 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1774,7 +1774,8 @@ static bool acpi_device_enumeration_by_parent(struct acpi_device *device)
}
void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
- int type, void (*release)(struct device *))
+ int type, void (*release)(struct device *),
+ struct list_head *mipi_img_head)
{
struct acpi_device *parent = acpi_find_parent_acpi_dev(handle);
@@ -1788,7 +1789,7 @@ void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
acpi_set_device_status(device, ACPI_STA_DEFAULT);
acpi_device_get_busid(device);
acpi_set_pnp_ids(handle, &device->pnp, type);
- acpi_init_properties(device);
+ acpi_init_properties(device, mipi_img_head);
acpi_bus_get_flags(device);
device->flags.match_driver = false;
device->flags.initialized = true;
@@ -1827,7 +1828,8 @@ static void acpi_scan_init_status(struct acpi_device *adev)
}
static int acpi_add_single_object(struct acpi_device **child,
- acpi_handle handle, int type, bool dep_init)
+ acpi_handle handle, int type, bool dep_init,
+ struct list_head *mipi_img_head)
{
struct acpi_device *device;
bool release_dep_lock = false;
@@ -1837,7 +1839,9 @@ static int acpi_add_single_object(struct acpi_device **child,
if (!device)
return -ENOMEM;
- acpi_init_device_object(device, handle, type, acpi_device_release);
+ acpi_init_device_object(device, handle, type, acpi_device_release,
+ mipi_img_head);
+
/*
* Getting the status is delayed till here so that we can call
* acpi_bus_get_status() and use its quirk handling. Note that
@@ -2048,16 +2052,20 @@ static u32 acpi_scan_check_dep(acpi_handle handle, bool check_dep)
}
/**
- * struct acpi_postponed_handle - A postponed ACPI handle
+ * struct acpi_postponed_handle - A postponed ACPI handle or device
* @list: Entry in a postponed list
* @handle: The postponed handle
+ * @device: The postponed device
*
- * One such entry represents an ACPI handle the scanning of which has been
- * postponed.
+ * One such entry represents an ACPI handle or an ACPI device the scanning of
+ * which has been postponed.
*/
struct acpi_postponed_handle {
struct list_head list;
- acpi_handle handle;
+ union {
+ acpi_handle handle;
+ struct acpi_device *device;
+ };
};
/**
@@ -2080,6 +2088,26 @@ static void acpi_bus_handle_postpone(acpi_handle handle, struct list_head *head)
list_add(&ph->list, head);
}
+/**
+ * acpi_bus_device_postpone - Add an ACPI device to a given postponed list
+ * @device: The ACPI device
+ * @head: Postponed list head
+ *
+ * Add a given ACPI device to a list of ACPI objects for which the creation
+ * of the device objects is to be postponed.
+ */
+void acpi_bus_device_postpone(struct acpi_device *device, struct list_head *head)
+{
+ struct acpi_postponed_handle *ph;
+
+ ph = kzalloc(sizeof(*ph), GFP_KERNEL);
+ if (!ph)
+ return;
+
+ ph->device = device;
+ list_add(&ph->list, head);
+}
+
static acpi_status acpi_bus_check_add(acpi_handle handle, bool first_pass,
struct acpi_scan_context *ctx)
{
@@ -2131,7 +2159,8 @@ static acpi_status acpi_bus_check_add(acpi_handle handle, bool first_pass,
* If first_pass is true at this point, the device has no dependencies,
* or the creation of the device object would have been postponed above.
*/
- acpi_add_single_object(&device, handle, type, !first_pass);
+ acpi_add_single_object(&device, handle, type, !first_pass,
+ &ctx->mipi_img_head);
if (!device)
return AE_CTRL_DEPTH;
@@ -2484,6 +2513,7 @@ int acpi_bus_scan(acpi_handle handle)
{
struct acpi_scan_context ctx = {
.postponed_head = LIST_HEAD_INIT(ctx.postponed_head),
+ .mipi_img_head = LIST_HEAD_INIT(ctx.mipi_img_head),
.csi2.crs_csi2_head = LIST_HEAD_INIT(ctx.csi2.crs_csi2_head),
};
struct acpi_postponed_handle *ph, *tmp_ph;
@@ -2517,13 +2547,25 @@ int acpi_bus_scan(acpi_handle handle)
acpi_bus_scan_crs_csi2(&ctx.csi2);
acpi_bus_scan_crs_csi2_release(&ctx.csi2.crs_csi2_head);
+ /*
+ * Initialise software nodes for devices that have MIPI DisCo for
+ * Imaging related properties.
+ */
+ list_for_each_entry_safe(ph, tmp_ph, &ctx.mipi_img_head, list) {
+ list_del(&ph->list);
+ acpi_init_swnodes(ph->device);
+ kfree(ph);
+ }
+
acpi_bus_attach(ctx.device, (void *)true);
/*
- * Proceed to register ACPI devices that were postponed due to _DEP
- * objects during the namespace walk.
+ * Proceed to register ACPI devices that were postponed due to _CRS CSI2
+ * resources or _DEP objects during the namespace walk.
*/
list_for_each_entry_safe(ph, tmp_ph, &ctx.postponed_head, list) {
+ struct acpi_postponed_handle *pm, *tmp_pm;
+
list_del(&ph->list);
/* Set device NULL here to obtain the root for this sub-tree. */
ctx.device = NULL;
@@ -2537,6 +2579,16 @@ int acpi_bus_scan(acpi_handle handle)
acpi_walk_namespace(ACPI_TYPE_ANY, ph->handle, ACPI_UINT32_MAX,
acpi_bus_check_add_2, NULL, (void *)&ctx,
NULL);
+ /*
+ * Initialise software nodes for devices that have MIPI DisCo
+ * for Imaging related properties but which were postponed
+ * because of _DEP.
+ */
+ list_for_each_entry_safe(pm, tmp_pm, &ctx.mipi_img_head, list) {
+ list_del(&pm->list);
+ acpi_init_swnodes(pm->device);
+ kfree(pm);
+ }
if (ctx.device)
acpi_bus_attach(ctx.device, NULL);
kfree(ph);
@@ -2597,7 +2649,7 @@ int acpi_bus_register_early_device(int type)
struct acpi_device *device = NULL;
int result;
- result = acpi_add_single_object(&device, NULL, type, false);
+ result = acpi_add_single_object(&device, NULL, type, false, NULL);
if (result)
return result;
@@ -2612,7 +2664,7 @@ static void acpi_bus_scan_fixed(void)
struct acpi_device *adev = NULL;
acpi_add_single_object(&adev, NULL, ACPI_BUS_TYPE_POWER_BUTTON,
- false);
+ false, NULL);
if (adev) {
adev->flags.match_driver = true;
if (device_attach(&adev->dev) >= 0)
@@ -2626,7 +2678,7 @@ static void acpi_bus_scan_fixed(void)
struct acpi_device *adev = NULL;
acpi_add_single_object(&adev, NULL, ACPI_BUS_TYPE_SLEEP_BUTTON,
- false);
+ false, NULL);
if (adev) {
adev->flags.match_driver = true;
if (device_attach(&adev->dev) < 0)
--
2.30.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v8 07/10] ACPI: property: Dig "rotation" property for devices with CSI2 _CRS
2023-03-29 10:09 [PATCH v8 00/10] ACPI _CRS CSI-2 and MIPI DisCo for Imaging support Sakari Ailus
` (5 preceding siblings ...)
2023-03-29 10:09 ` [PATCH v8 06/10] ACPI: scan: Generate software nodes based on MIPI " Sakari Ailus
@ 2023-03-29 10:09 ` Sakari Ailus
2023-03-29 10:09 ` [PATCH v8 08/10] ACPI: property: Rename parsed MIPI DisCo for Imaging properties Sakari Ailus
` (2 subsequent siblings)
9 siblings, 0 replies; 36+ messages in thread
From: Sakari Ailus @ 2023-03-29 10:09 UTC (permalink / raw)
To: linux-acpi; +Cc: linux-media, rafael, andriy.shevchenko, heikki.krogerus
Dig "rotation" property value for devices with _CRS CSI2 resource
descriptor. The value comes from _PLD (physical location of device)
object, if it exists for the device.
This way camera sensor drivers that know the "rotation" property do not
need to care about _PLD on ACPI.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/acpi/mipi.c | 17 +++++++++++++++++
include/acpi/acpi_bus.h | 1 +
2 files changed, 18 insertions(+)
diff --git a/drivers/acpi/mipi.c b/drivers/acpi/mipi.c
index 03079f78bd2c..0232cc123ead 100644
--- a/drivers/acpi/mipi.c
+++ b/drivers/acpi/mipi.c
@@ -652,10 +652,12 @@ static void init_port_csi2_remote(struct acpi_device *device,
*/
void acpi_init_swnodes(struct acpi_device *device)
{
+ struct fwnode_handle *fwnode = acpi_fwnode_handle(device);
struct acpi_device_software_nodes *ads;
struct acpi_buffer buffer = { .length = ACPI_ALLOCATE_BUFFER };
acpi_handle handle = acpi_device_handle(device);
struct fwnode_handle *primary;
+ unsigned int prop_index = 0;
acpi_status status;
unsigned int i;
int ret;
@@ -664,6 +666,21 @@ void acpi_init_swnodes(struct acpi_device *device)
if (!ads)
return;
+ /*
+ * Check if "rotation" property exists and if it doesn't but there's a
+ * _PLD object, then get the rotation value from there.
+ */
+ if (!fwnode_property_present(fwnode, "rotation")) {
+ struct acpi_pld_info *pld;
+
+ status = acpi_get_physical_device_location(handle, &pld);
+ if (ACPI_SUCCESS(status)) {
+ ads->dev_props[NEXT_PROPERTY(prop_index, DEV_ROTATION)] =
+ PROPERTY_ENTRY_U32("rotation", pld->rotation * 45U);
+ kfree(pld);
+ }
+ }
+
status = acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
if (ACPI_FAILURE(status)) {
acpi_handle_warn(handle, "cannot get path name\n");
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 29d1e4d49d0c..83a89797a122 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -361,6 +361,7 @@ struct acpi_device_data {
struct acpi_gpio_mapping;
enum acpi_device_swnode_dev_props {
+ ACPI_DEVICE_SWNODE_DEV_ROTATION,
ACPI_DEVICE_SWNODE_DEV_NUM_OF,
ACPI_DEVICE_SWNODE_DEV_NUM_ENTRIES
};
--
2.30.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v8 08/10] ACPI: property: Rename parsed MIPI DisCo for Imaging properties
2023-03-29 10:09 [PATCH v8 00/10] ACPI _CRS CSI-2 and MIPI DisCo for Imaging support Sakari Ailus
` (6 preceding siblings ...)
2023-03-29 10:09 ` [PATCH v8 07/10] ACPI: property: Dig "rotation" property for devices with CSI2 _CRS Sakari Ailus
@ 2023-03-29 10:09 ` Sakari Ailus
2023-05-19 18:34 ` Rafael J. Wysocki
2023-03-29 10:09 ` [PATCH v8 09/10] ACPI: property: Skip MIPI property table without "mipi-img" prefix Sakari Ailus
2023-03-29 10:09 ` [PATCH v8 10/10] ACPI: property: Document _CRS CSI-2 and DisCo for Imaging support Sakari Ailus
9 siblings, 1 reply; 36+ messages in thread
From: Sakari Ailus @ 2023-03-29 10:09 UTC (permalink / raw)
To: linux-acpi; +Cc: linux-media, rafael, andriy.shevchenko, heikki.krogerus
MIPI DisCo for Imaging defines properties for sensor-adjacent devices such
as EEPROM, LED flash or lens VCM as either device or sub-node references.
This is compliant with existing DT definitions apart from property names.
Rename parsed MIPI-defined properties so drivers will have a unified view
of them as defined in DT and already parsed by drivers. This can be done
in-place as the MIPI-defined property strings are always longer than the
DT one. This also results in loss of constness in parser function
arguments.
Individual bindings to devices could define the references differently
between MIPI DisCo for Imaging and DT, in terms of device or sub-node
references. This will still need to be handled in the drivers themselves.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/acpi/internal.h | 1 +
drivers/acpi/mipi.c | 36 ++++++++++++++++++++++++++++++++++++
drivers/acpi/property.c | 22 ++++++++++++----------
3 files changed, 49 insertions(+), 10 deletions(-)
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 36799cde281c..ef5af9fd9ffa 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -318,5 +318,6 @@ void acpi_bus_scan_check_crs_csi2(acpi_handle handle, struct acpi_scan_context *
void acpi_bus_scan_crs_csi2_release(struct list_head *crs_csi2_handles);
void acpi_bus_scan_crs_csi2(struct acpi_scan_context_csi2 *ctx);
void acpi_init_swnodes(struct acpi_device *device);
+void acpi_properties_prepare_mipi(union acpi_object *elements);
#endif /* _ACPI_INTERNAL_H_ */
diff --git a/drivers/acpi/mipi.c b/drivers/acpi/mipi.c
index 0232cc123ead..576301b9bb89 100644
--- a/drivers/acpi/mipi.c
+++ b/drivers/acpi/mipi.c
@@ -714,3 +714,39 @@ void acpi_init_swnodes(struct acpi_device *device)
primary = acpi_fwnode_handle(device);
primary->secondary = software_node_fwnode(ads->nodes);
}
+
+static const struct mipi_disco_prop {
+ const char *mipi_prop;
+ const char *dt_prop;
+} mipi_disco_props[] = {
+ { "mipi-img-lens-focus", "lens-focus" },
+ { "mipi-img-flash-leds", "flash-leds" },
+ { "mipi-img-clock-frequency", "clock-frequency" },
+ { "mipi-img-led-max-current", "led-max-microamp" },
+ { "mipi-img-flash-max-current", "flash-max-microamp" },
+ { "mipi-img-flash-max-timeout", "flash-max-timeout-us" },
+};
+
+/**
+ * acpi_properties_prepare_mipi - Rename MIPI properties as commin DT ones
+ *
+ * @elements: ACPI object containing _DSD properties for a device node
+ *
+ * Renames MIPI-defined properties as common DT ones. The pre-requisite is that
+ * the names of all such MIPI properties are no longer than the corresponding DT
+ * ones.
+ */
+void acpi_properties_prepare_mipi(union acpi_object *elements)
+{
+ char *e0 = elements[0].string.pointer;
+ unsigned int i;
+
+ /* Replace MIPI DisCo for Imaging property names with DT equivalents. */
+ for (i = 0; i < ARRAY_SIZE(mipi_disco_props); i++) {
+ if (!strcmp(mipi_disco_props[i].mipi_prop, e0)) {
+ WARN_ON(strscpy(e0, mipi_disco_props[i].dt_prop,
+ elements[0].string.length) < 0);
+ break;
+ }
+ }
+}
diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 1892787e73a6..541bda2c118f 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -127,7 +127,7 @@ static bool acpi_nondev_subnode_extract(union acpi_object *desc,
}
static bool acpi_nondev_subnode_data_ok(acpi_handle handle,
- const union acpi_object *link,
+ union acpi_object *link,
struct list_head *list,
struct fwnode_handle *parent)
{
@@ -148,7 +148,7 @@ static bool acpi_nondev_subnode_data_ok(acpi_handle handle,
}
static bool acpi_nondev_subnode_ok(acpi_handle scope,
- const union acpi_object *link,
+ union acpi_object *link,
struct list_head *list,
struct fwnode_handle *parent)
{
@@ -292,22 +292,24 @@ static bool acpi_property_value_ok(const union acpi_object *value)
return false;
}
-static bool acpi_properties_format_valid(const union acpi_object *properties)
+static bool acpi_properties_prepare(union acpi_object *properties)
{
- int i;
+ unsigned int i;
for (i = 0; i < properties->package.count; i++) {
- const union acpi_object *property;
+ union acpi_object *property = &properties->package.elements[i];
+ union acpi_object *elements = property->package.elements;
- property = &properties->package.elements[i];
/*
* Only two elements allowed, the first one must be a string and
* the second one has to satisfy certain conditions.
*/
- if (property->package.count != 2
- || property->package.elements[0].type != ACPI_TYPE_STRING
- || !acpi_property_value_ok(&property->package.elements[1]))
+ if (property->package.count != 2 ||
+ elements[0].type != ACPI_TYPE_STRING ||
+ !acpi_property_value_ok(&elements[1]))
return false;
+
+ acpi_properties_prepare_mipi(elements);
}
return true;
}
@@ -539,7 +541,7 @@ static bool acpi_extract_properties(acpi_handle scope, union acpi_object *desc,
* We found the matching GUID. Now validate the format of the
* package immediately following it.
*/
- if (!acpi_properties_format_valid(properties))
+ if (!acpi_properties_prepare(properties))
continue;
acpi_data_add_props(data, (const guid_t *)guid->buffer.pointer,
--
2.30.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v8 08/10] ACPI: property: Rename parsed MIPI DisCo for Imaging properties
2023-03-29 10:09 ` [PATCH v8 08/10] ACPI: property: Rename parsed MIPI DisCo for Imaging properties Sakari Ailus
@ 2023-05-19 18:34 ` Rafael J. Wysocki
2023-05-19 18:42 ` Sakari Ailus
0 siblings, 1 reply; 36+ messages in thread
From: Rafael J. Wysocki @ 2023-05-19 18:34 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-acpi, linux-media, rafael, andriy.shevchenko,
heikki.krogerus
On Wed, Mar 29, 2023 at 12:10 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> MIPI DisCo for Imaging defines properties for sensor-adjacent devices such
> as EEPROM, LED flash or lens VCM as either device or sub-node references.
> This is compliant with existing DT definitions apart from property names.
>
> Rename parsed MIPI-defined properties so drivers will have a unified view
> of them as defined in DT and already parsed by drivers.
I don't particularly like this idea.
One of the drawbacks is that if somebody doesn't care about DT
bindings (for instance, because they will always run on platforms
without DT), they won't be able to use the MIPI-defined property names
in their code.
I would very much prefer to add a set of DT-defined properties with
the same values. The, whoever wants to use the property names from
the DT bindings, they will be able to do that, but it will be also
possible to use the MIPI-defined ones.
The previous patch adds the "rotation" property to the swnodes set, so
I don't see any problems with doing that for the properties in
question.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v8 08/10] ACPI: property: Rename parsed MIPI DisCo for Imaging properties
2023-05-19 18:34 ` Rafael J. Wysocki
@ 2023-05-19 18:42 ` Sakari Ailus
0 siblings, 0 replies; 36+ messages in thread
From: Sakari Ailus @ 2023-05-19 18:42 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: linux-acpi, linux-media, andriy.shevchenko, heikki.krogerus
Hi Rafael,
On Fri, May 19, 2023 at 08:34:34PM +0200, Rafael J. Wysocki wrote:
> On Wed, Mar 29, 2023 at 12:10 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > MIPI DisCo for Imaging defines properties for sensor-adjacent devices such
> > as EEPROM, LED flash or lens VCM as either device or sub-node references.
> > This is compliant with existing DT definitions apart from property names.
> >
> > Rename parsed MIPI-defined properties so drivers will have a unified view
> > of them as defined in DT and already parsed by drivers.
>
> I don't particularly like this idea.
>
> One of the drawbacks is that if somebody doesn't care about DT
> bindings (for instance, because they will always run on platforms
> without DT), they won't be able to use the MIPI-defined property names
> in their code.
>
> I would very much prefer to add a set of DT-defined properties with
> the same values. The, whoever wants to use the property names from
> the DT bindings, they will be able to do that, but it will be also
> possible to use the MIPI-defined ones.
>
> The previous patch adds the "rotation" property to the swnodes set, so
> I don't see any problems with doing that for the properties in
> question.
I don't think this would be a problem really, no, but I question the need
to ever access the MIPI specification names in Linux outside this piece of
code. Drivers for cameras, lens controllers and LED flashes generally try
to avoid being specific to a given firmware interface and the established
de facto naming of these properties in the kernel is aligned with
Devicetree.
I'd like to see differences only when the functionality differs, otherwise
they should be the same. Creating a copy when you can modify it is waste of
a bit of memory. On the upside, the object memory could remain const that
way.
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v8 09/10] ACPI: property: Skip MIPI property table without "mipi-img" prefix
2023-03-29 10:09 [PATCH v8 00/10] ACPI _CRS CSI-2 and MIPI DisCo for Imaging support Sakari Ailus
` (7 preceding siblings ...)
2023-03-29 10:09 ` [PATCH v8 08/10] ACPI: property: Rename parsed MIPI DisCo for Imaging properties Sakari Ailus
@ 2023-03-29 10:09 ` Sakari Ailus
2023-03-29 10:09 ` [PATCH v8 10/10] ACPI: property: Document _CRS CSI-2 and DisCo for Imaging support Sakari Ailus
9 siblings, 0 replies; 36+ messages in thread
From: Sakari Ailus @ 2023-03-29 10:09 UTC (permalink / raw)
To: linux-acpi; +Cc: linux-media, rafael, andriy.shevchenko, heikki.krogerus
For all _DSD properties, skip going through the MIPI DisCo for Imaging
property name substitution table if the property doesn't have "mipi-img-"
prefix.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/acpi/mipi.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/acpi/mipi.c b/drivers/acpi/mipi.c
index 576301b9bb89..90ad4b43f008 100644
--- a/drivers/acpi/mipi.c
+++ b/drivers/acpi/mipi.c
@@ -715,6 +715,8 @@ void acpi_init_swnodes(struct acpi_device *device)
primary->secondary = software_node_fwnode(ads->nodes);
}
+#define MIPI_IMG_PREFIX "mipi-img-"
+
static const struct mipi_disco_prop {
const char *mipi_prop;
const char *dt_prop;
@@ -741,6 +743,9 @@ void acpi_properties_prepare_mipi(union acpi_object *elements)
char *e0 = elements[0].string.pointer;
unsigned int i;
+ if (!str_has_prefix(elements[0].string.pointer, MIPI_IMG_PREFIX))
+ return;
+
/* Replace MIPI DisCo for Imaging property names with DT equivalents. */
for (i = 0; i < ARRAY_SIZE(mipi_disco_props); i++) {
if (!strcmp(mipi_disco_props[i].mipi_prop, e0)) {
--
2.30.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v8 10/10] ACPI: property: Document _CRS CSI-2 and DisCo for Imaging support
2023-03-29 10:09 [PATCH v8 00/10] ACPI _CRS CSI-2 and MIPI DisCo for Imaging support Sakari Ailus
` (8 preceding siblings ...)
2023-03-29 10:09 ` [PATCH v8 09/10] ACPI: property: Skip MIPI property table without "mipi-img" prefix Sakari Ailus
@ 2023-03-29 10:09 ` Sakari Ailus
9 siblings, 0 replies; 36+ messages in thread
From: Sakari Ailus @ 2023-03-29 10:09 UTC (permalink / raw)
To: linux-acpi; +Cc: linux-media, rafael, andriy.shevchenko, heikki.krogerus
Document how ACPI _CRS CSI-2 and DisCo for Imaging works. It's non-trivial
so such documentation can be useful.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/acpi/mipi.c | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/drivers/acpi/mipi.c b/drivers/acpi/mipi.c
index 90ad4b43f008..926d39948268 100644
--- a/drivers/acpi/mipi.c
+++ b/drivers/acpi/mipi.c
@@ -9,6 +9,43 @@
* Also see <URL:https://www.mipi.org/specifications/mipi-disco-imaging>.
*
* Copyright (C) 2023 Intel Corporation
+ *
+ * _CRS CSI-2 descriptors, as defined starting from ACPI 6.4 [1], contain
+ * information on cross-device CSI-2 bus configuration. The descriptors are
+ * located under transmitter devices, and the receiver devices have no direct
+ * way to access them even if the information in these descriptors is equally
+ * important for receivers. This information is amended with MIPI DisCo for
+ * Imaging [2] specification that defines _DSD data nodes and properties.
+ *
+ * The support for these is based on two-fold approach, firstly renaming
+ * properties where semantics matches and secondly gathering information to
+ * generate properties using information gathered from various sources. The
+ * former is trivial (see acpi_properties_prepare_mipi() at the end of the
+ * file) whereas the latter requires a more elaborate explanation.
+ *
+ * acpi_bus_scan_crs_csi2() scans an ACPI bus for devices with _CRS CSI-2
+ * descriptors and stores them to a linked list. This is done as traversing just
+ * this list is much smaller task than the entire DSDT. This list is then used
+ * to figure out how much memory is needed for swnodes related to a given ACPI
+ * device (handle). Further on, the same function sets the property values for
+ * the properties the values of which are obtained from the _CRS CSI-2
+ * descriptor. The information is stored into another list where the information
+ * can be looked up based on device's acpi_handle as the struct acpi_device
+ * isn't available yet at this point (and could not, as cross-device references
+ * need to be set up before the devices are available for drivers to probe).
+ *
+ * For each struct acpi_device, acpi_init_swnodes() further obtains information
+ * required to find out the values for the rest of the properties needed by
+ * drivers. This includes all port and endpoint properties as the node
+ * structures used by DT graphs and DisCo for Imaging are different. Finally the
+ * function registers software nodes for the device and sets the secondary
+ * pointer for the ACPI device's fwnode.
+ *
+ * Access to data the structures is serialised using acpi_scan_lock in scan.c.
+ *
+ * [1] https://uefi.org/sites/default/files/resources/ACPI_Spec_6_4_Jan22.pdf
+ *
+ * [2] https://www.mipi.org/specifications/mipi-disco-imaging
*/
#include <linux/acpi.h>
--
2.30.2
^ permalink raw reply related [flat|nested] 36+ messages in thread