Re: [PATCH v2 02/12] media: v4l: fwnode: Support ACPI's _PLD for v4l2_fwnode_device_parse

On Mon, 14 Jul 2025 at 15:03, Hans de Goede [off-list ref] wrote:

Hi,

On 5-Jun-25 19:52, Ricardo Ribalda wrote:

quoted

Currently v4l2_fwnode_device_parse() obtains the orientation and
rotation via fwnode properties.

Extend the function to support as well ACPI devices with _PLD info.

We give a higher priority to fwnode, because it might contain quirks
injected via swnodes.

Signed-off-by: Ricardo Ribalda <redacted>
---
 drivers/media/v4l2-core/v4l2-fwnode.c | 85 ++++++++++++++++++++++++++++++++---
 1 file changed, 79 insertions(+), 6 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index cb153ce42c45d69600a3ec4e59a5584d7e791a2a..379290ab3cfde74c8f663d61837a9a95011b5ae0 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,65 @@ 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 (!is_acpi_device_node(dev_fwnode(dev)))
+             return 0;
+
+     if (!acpi_get_physical_device_location(ACPI_HANDLE(dev), &pld)) {
+             dev_dbg(dev, "acpi _PLD call failed\n");
+             return 0;
+     }
+
+     if (props->orientation != V4L2_FWNODE_PROPERTY_UNSET) {

"!=" should be "==" here. So that we set it when not set already,
rather then leaving it unset when not set already.

quoted

+             switch (pld->panel) {
+             case ACPI_PLD_PANEL_FRONT:
+                     props->orientation = V4L2_FWNODE_ORIENTATION_FRONT;
+                     break;
+             case ACPI_PLD_PANEL_BACK:
+                     props->orientation = V4L2_FWNODE_ORIENTATION_BACK;
+                     break;
+             case ACPI_PLD_PANEL_TOP:
+             case ACPI_PLD_PANEL_LEFT:
+             case ACPI_PLD_PANEL_RIGHT:
+             case ACPI_PLD_PANEL_UNKNOWN:
+                     props->orientation = V4L2_FWNODE_ORIENTATION_EXTERNAL;
+                     break;
+             default:
+                     dev_dbg(dev, "Unknown _PLD panel val %d\n", pld->panel);
+                     ret = -EINVAL;
+                     goto done;
+             }
+     }
+
+     if (props->rotation != V4L2_FWNODE_PROPERTY_UNSET) {

Same here.

quoted

+             switch (pld->rotation) {
+             case 0 ... 7:
+                     props->rotation = pld->rotation * 45;
+                     break;
+             default:
+                     dev_dbg(dev, "Unknown _PLD rotation val %d\n", pld->panel);
+                     ret = -EINVAL;
+                     goto done;
+             }
+     }
+
+done:
+     ACPI_FREE(pld);
+     return ret;
+}
+
+static int v4l2_fwnode_device_parse_dt(struct device *dev,
+                                    struct v4l2_fwnode_device_properties *props)
 {
      struct fwnode_handle *fwnode = dev_fwnode(dev);
      u32 val;
      int ret;

-     memset(props, 0, sizeof(*props));
-
-     props->orientation = V4L2_FWNODE_PROPERTY_UNSET;
      ret = fwnode_property_read_u32(fwnode, "orientation", &val);
      if (!ret) {
              switch (val) {

@@ -833,7 +883,6 @@ int v4l2_fwnode_device_parse(struct device *dev,
              dev_dbg(dev, "device orientation: %u\n", val);
      }

-     props->rotation = V4L2_FWNODE_PROPERTY_UNSET;
      ret = fwnode_property_read_u32(fwnode, "rotation", &val);
      if (!ret) {
              if (val >= 360) {

@@ -847,6 +896,30 @@ int v4l2_fwnode_device_parse(struct device *dev,

      return 0;
 }
+
+int v4l2_fwnode_device_parse(struct device *dev,
+                          struct v4l2_fwnode_device_properties *props)
+{
+     int ret;
+
+     memset(props, 0, sizeof(*props));
+
+     props->orientation = V4L2_FWNODE_PROPERTY_UNSET;
+     props->rotation = V4L2_FWNODE_PROPERTY_UNSET;
+
+     /* Start by looking into swnodes and dt. */
+     ret =  v4l2_fwnode_device_parse_dt(dev, props);
+     if (ret)
+             return ret;
+
+     /* Orientation and rotation found!, we are ready. */
+     if (props->orientation != V4L2_FWNODE_PROPERTY_UNSET &&
+         props->rotation != V4L2_FWNODE_PROPERTY_UNSET)
+             return 0;

As Sakari set, this can be dropped, with the 2 checks above
fixed to be == v4l2_fwnode_device_parse_acpi() will become
a no-op in this case.

I wanted to avoid calling he _PLD method if it was not necessary.

But if both Sakari and you dislike the optimization then let's get rid of it :)

Thanks

With these things fixed:

Reviewed-by: Hans de Goede <hansg@kernel.org>

Regards,

Hans

quoted

+
+     /* Let's check the acpi table. */
+     return v4l2_fwnode_device_parse_acpi(dev, props);
+}
 EXPORT_SYMBOL_GPL(v4l2_fwnode_device_parse);

 /*

--
Ricardo Ribalda