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@xxxxxx> 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@xxxxxxxxxxxx>
>>> ---
>>> 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@xxxxxx>
>>> */
>>> #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
