Re: [PATCH v5 2/8] ACPI: property: Parse _CRS CSI-2 descriptor

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux