Re: [PATCH 3/8] media: v4l: fwnode: Support acpi devices for v4l2_fwnode_device_parse

[Sakari Ailus <sakari.ailus@iki.fi>]

Hi Hans, Ricardo,

On Tue, Apr 22, 2025 at 10:44:41AM +0200, Hans de Goede wrote:
> Hi Ricardo,
> 
> On 22-Apr-25 2:23 AM, Ricardo Ribalda wrote:
> > Hi Sakari
> > 
> > On Sun, 13 Apr 2025 at 17:50, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> >>
> >> Hi Ricardo,
> >>
> >> Thanks for the patch.
> >>
> >> On Thu, Apr 03, 2025 at 07:16:14PM +0000, Ricardo Ribalda wrote:
> >>> This patch modifies v4l2_fwnode_device_parse() to support ACPI devices.
> >>>
> >>> We initially add support only for orientation via the ACPI _PLD method.
> >>>
> >>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> >>> ---
> >>>  drivers/media/v4l2-core/v4l2-fwnode.c | 58 +++++++++++++++++++++++++++++++----
> >>>  1 file changed, 52 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> >>> index cb153ce42c45d69600a3ec4e59a5584d7e791a2a..81563c36b6436bb61e1c96f2a5ede3fa9d64dab3 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> >>> @@ -15,6 +15,7 @@
> >>>   * Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> >>>   */
> >>>  #include <linux/acpi.h>
> >>> +#include <acpi/acpi_bus.h>
> >>>  #include <linux/kernel.h>
> >>>  #include <linux/mm.h>
> >>>  #include <linux/module.h>
> >>> @@ -807,16 +808,47 @@ int v4l2_fwnode_connector_add_link(struct fwnode_handle *fwnode,
> >>>  }
> >>>  EXPORT_SYMBOL_GPL(v4l2_fwnode_connector_add_link);
> >>>
> >>> -int v4l2_fwnode_device_parse(struct device *dev,
> >>> -                          struct v4l2_fwnode_device_properties *props)
> >>> +static int v4l2_fwnode_device_parse_acpi(struct device *dev,
> >>> +                                      struct v4l2_fwnode_device_properties *props)
> >>> +{
> >>> +     struct acpi_pld_info *pld;
> >>> +     int ret = 0;
> >>> +
> >>> +     if (!acpi_get_physical_device_location(ACPI_HANDLE(dev), &pld)) {
> >>> +             dev_dbg(dev, "acpi _PLD call failed\n");
> >>> +             return 0;
> >>> +     }
> >>
> >> You could have software nodes in an ACPI system as well as DT-aligned
> >> properties. They're not the primary means to convey this information still.
> >>
> >> How about returning e.g. -ENODATA here if _PLD doesn't exist for the device
> >> and then proceeding to parse properties as in DT?
> > 
> > Do you mean that there can be devices with ACPI handles that can also
> > have DT properties?
> 
> Yes it is possible to embed DT properties in ACPI, but I don't
> think that is really applicable here.

This is determined by
Documentation/firmware-guide/acpi/DSD-properties-rules.rst . So rotation
and orientation shouldn't come from _DSD properties on ACPI systems.

> 
> But we also have secondary software-fwnodes which are used
> extensively on x86 to set device-properties on devices by
> platform code to deal with ACPI tables sometimes having
> incomplete information.
> 
> For example atm _PLD is already being parsed in:
> 
> drivers/media/pci/intel/ipu-bridge.c and that is then used to add
> a standard "orientation" device-property on the sensor device.
> 
> This is actually something which I guess we can drop once your
> patches are in, since those should then do the same in a more
> generic manner.

DisCo for Imaging support currently also digs this information from _PDL
(see init_crs_csi2_swnodes() in drivers/acpi/mipi-disco-img.c), but this
is only done if a _CRS CSI-2 descriptor is present. It could also be done
for devices with the IPU Windows specific ACPI objects and it would be a
natural location for handing quirks -- there are some
unrelated Dell DSDT quirks already.

> 
> > What shall we do if _PLD contradicts the DT property? What takes precedence?
> 
> As for priorities, at east for rotation it seems that we are going
> to need some quirks, I already have a few Dell laptops where it seems
> that the sensor is upside down and parsing the rotation field in
> the IPU6 specific SSDB ACPI package does not yield a 180° rotation,
> so we are going to need some quirks.
> 
> I expect these quirks to live in the bridge code, while your helper
> will be called from sensor drivers, so in order to allow quirks to
> override things, I think that first the "orientation" device-property
> should be checked (which the ACPI glue code we have can set before
> the sensor driver binds) and only then should _PLD be checked.
> 
> IOW _PLD should be seen as the fallback, because ACPI tables are
> often a copy and paste job so it can very well contain wrong info
> copy-pasted from some example ACPI code or from another hw model.

Unfortunately that does happen. :-(

-- 
Regards,

Sakari Ailus