Hi Rafael,
On Tue, Mar 07, 2023 at 02:38:43PM +0100, Rafael J. Wysocki wrote:
> Note: The report from Dan Carpenter has not been addressed.
I'll reply to Dan --- it's a false positive.
>
> On Wed, Feb 8, 2023 at 10:27 PM Sakari Ailus
> <sakari.ailus@xxxxxxxxxxxxxxx> wrote:
> >
> > Parse newly added ACPI _CRS CSI-2 descriptor for CSI-2 and camera
> > configuration. For now, only figure out where the descriptor is present in
> > order to allow adding information from it to related devices.
>
> This should contain (a) pointer(s) to the relevant specification material.
I can add that for v6.
>
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > ---
> > drivers/acpi/Makefile | 2 +-
> > drivers/acpi/internal.h | 7 +
> > drivers/acpi/mipi.c | 358 ++++++++++++++++++++++++++++++++++++++++
> > drivers/acpi/scan.c | 16 +-
> > include/acpi/acpi_bus.h | 11 ++
> > 5 files changed, 389 insertions(+), 5 deletions(-)
> > create mode 100644 drivers/acpi/mipi.c
> >
> > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> > index a5b649e71ab1b..a98fa1bc15548 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 ec584442fb298..1ec4aa92bf17a 100644
> > --- a/drivers/acpi/internal.h
> > +++ b/drivers/acpi/internal.h
> > @@ -282,4 +282,11 @@ void acpi_init_lpit(void);
> > static inline void acpi_init_lpit(void) { }
> > #endif
> >
> > +/*--------------------------------------------------------------------------
> > + ACPI and MIPI DisCo for Imaging conversion
> > + -------------------------------------------------------------------------- */
> > +
> > +void acpi_crs_csi2_swnodes_del_free(void);
> > +void acpi_bus_scan_crs_csi2(acpi_handle handle);
> > +
> > #endif /* _ACPI_INTERNAL_H_ */
> > diff --git a/drivers/acpi/mipi.c b/drivers/acpi/mipi.c
> > new file mode 100644
> > index 0000000000000..33d50df831933
> > --- /dev/null
> > +++ b/drivers/acpi/mipi.c
> > @@ -0,0 +1,358 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * MIPI DisCo for Imaging support.
>
> I do realize that the last patch in the series contains the
> description of this code, but it would be really helpful to put at
> least some of it here and update it in the following patches as the
> code gets modified.
I'll see if it could be meaningfully split, with some going to this patch.
>
> > + *
> > + * Copyright (C) 2023 Intel Corporation
> > + */
...
> > +void acpi_bus_scan_crs_csi2(acpi_handle handle)
> > +{
> > + struct acpi_handle_ref *handle_refs;
> > + struct acpi_handle_ref *this = NULL;
> > + struct crs_csi2_all csi2_all = {
> > + .crs_csi2_head = LIST_HEAD_INIT(csi2_all.crs_csi2_head),
> > + };
> > + size_t this_count;
> > + unsigned int curr = 0;
> > + struct crs_csi2 *csi2;
> > +
> > + /*
> > + * Collect the devices that have a _CRS CSI-2 resource. Each CSI-2 TX
> > + * port has one.
> > + */
> > + acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, ACPI_UINT32_MAX,
> > + scan_check_crs_csi2, NULL, &csi2_all, NULL);
>
> I still don't like this walk and I still think that it is avoidable.
>
> At least I don't see why it isn't avoidable ATM.
In acpi_scan_init(), the ACPI tables has been just obtained (via
acpi_get_table()), and the second table traversal in acpi_bus_scan(),
acpi_bus_check_add_1() will, in turn, create devices that further can be
probed.
I guess we could look at the "mipi-img-port-*" nodes right under the device
to see whether the device is expected to be a CSI-2 receiver. I'm not sure
if this would bring a performance improvement or further degradation.
Another option could be to opportunistically initialise a device, and once
encountering a "mipi-img-port-*" node in _DSD property parsing, bail out
and postpone initialisation of the device on first namespace traversal.
This would mean adding some extra error handling in e.g.
acpi_init_device_object() callers.
As the proportion of devices with CSI-2 connections compared to all devices
is expected to be rather small, the second option would likely bring a
performance improvement compared to the current implementation but it would
be somewhat ugly to involve property parsing with this IMO.
--
Kind regards,
Sakari Ailus
