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

From: Sakari Ailus <sakari.ailus@linux.intel.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: linux-acpi@vger.kernel.org, linux-media@vger.kernel.org,
	rafael@kernel.org, heikki.krogerus@linux.intel.com
Subject: Re: [PATCH v7 03/10] ACPI: property: Parse _CRS CSI-2 descriptor
Date: Tue, 28 Mar 2023 18:27:09 +0300	[thread overview]
Message-ID: <ZCMHTZgXnLCWjCje@kekkonen.localdomain> (raw)
In-Reply-To: <ZCMDyVoFWizx+tOp@smile.fi.intel.com>

Hi Andy,

Thank you for the review.

On Tue, Mar 28, 2023 at 06:12:09PM +0300, Andy Shevchenko wrote:
> On Tue, Mar 28, 2023 at 01:12:56PM +0300, 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.
> 
> ...
> 
> > +struct acpi_scan_context {
> > +	struct acpi_device *device;
> > +	struct list_head postponed_head;
> 
> Make it first?

Soon this isn't the only list here, only one of them can be first. But I
guess there is some benefit nonetheless.

> 
> > +	struct acpi_scan_context_csi2 csi2;
> > +};
> 
> ...
> 
> > +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)
> 
> Who is going to release resources allocated in the callback above?

This is done by crs_csi2_release(), called from acpi_bus_scan_crs_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;
> > +}
> 
> ...
> 
> > +	/*
> > +	 * Allocate memory for ports, node pointers (number of nodes +
> > +	 * 1 (guardian), nodes (root + number of ports * 2 (for for
> > +	 * every port there is an endpoint)).
> > +	 */
> 
> for for ?
> 
> I am a bit lost here. Can you put the above in more mathematical language?

The first "for" is in the sense of "because". I can replace it if you think
it'd be clearer that way. 

There is simply a single endpoint for every port, as DisCo for Imaging does
not support the notion of endpoints (where you could have multiple
connections with more endpoints).

-- 
Kind regards,

Sakari Ailus