[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CANiDSCvQC25ZrSZgUuFt6deCogFL6=GPsYYrsegK1NOK=uzRJA@mail.gmail.com>
Date: Tue, 22 Apr 2025 08:23:52 +0800
From: Ricardo Ribalda <ribalda@...omium.org>
To: Sakari Ailus <sakari.ailus@....fi>
Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>, Hans de Goede <hdegoede@...hat.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>, Hans Verkuil <hverkuil@...all.nl>,
Sakari Ailus <sakari.ailus@...ux.intel.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Linus Walleij <linus.walleij@...aro.org>, Bartosz Golaszewski <brgl@...ev.pl>, linux-media@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
devicetree@...r.kernel.org, linux-gpio@...r.kernel.org
Subject: Re: [PATCH 3/8] media: v4l: fwnode: Support acpi devices for v4l2_fwnode_device_parse
Hi Sakari
On Sun, 13 Apr 2025 at 17:50, Sakari Ailus <sakari.ailus@....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@...omium.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@....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
--
Ricardo Ribalda
Powered by blists - more mailing lists