Re: [PATCH v8 03/10] ACPI: property: Parse _CRS CSI-2 descriptor

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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