| Message ID | 20250403-uvc-orientation-v1-3-1a0cc595a62d@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | media: uvcvideo: Add support for V4L2_CID_CAMERA_SENSOR_ORIENTATION | expand |
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? > + > + 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; How about the rotation in _PLD? > + default: > + dev_dbg(dev, "Unknown _PLD panel val %d\n", pld->panel); > + ret = -EINVAL; > + break; > + } > + > + 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 +865,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 +878,21 @@ int v4l2_fwnode_device_parse(struct device *dev, > > return 0; > } > + > +int v4l2_fwnode_device_parse(struct device *dev, > + struct v4l2_fwnode_device_properties *props) > +{ > + struct fwnode_handle *fwnode = dev_fwnode(dev); > + > + memset(props, 0, sizeof(*props)); > + > + props->orientation = V4L2_FWNODE_PROPERTY_UNSET; > + props->rotation = V4L2_FWNODE_PROPERTY_UNSET; > + > + if (is_acpi_device_node(fwnode)) > + return v4l2_fwnode_device_parse_acpi(dev, props); > + return v4l2_fwnode_device_parse_dt(dev, props); > +} > EXPORT_SYMBOL_GPL(v4l2_fwnode_device_parse); > > /* >
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. 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. > 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. Regards, Hans
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; + } + + 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; + break; + } + + 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 +865,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 +878,21 @@ int v4l2_fwnode_device_parse(struct device *dev, return 0; } + +int v4l2_fwnode_device_parse(struct device *dev, + struct v4l2_fwnode_device_properties *props) +{ + struct fwnode_handle *fwnode = dev_fwnode(dev); + + memset(props, 0, sizeof(*props)); + + props->orientation = V4L2_FWNODE_PROPERTY_UNSET; + props->rotation = V4L2_FWNODE_PROPERTY_UNSET; + + if (is_acpi_device_node(fwnode)) + return v4l2_fwnode_device_parse_acpi(dev, props); + return v4l2_fwnode_device_parse_dt(dev, props); +} EXPORT_SYMBOL_GPL(v4l2_fwnode_device_parse); /*
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(-)