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
