| 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 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? Wow that is new to me :). What shall we do if _PLD contradicts the DT property? What takes precedence? > > > + > > + 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? If we agree on the orientation part I will extend it to support rotation. It should not be a complicated change. > > > + 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); > > > > /* > > > > -- > Kind regards, > > Sakari Ailus
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(-)