Hi, On 1/16/22 23:20, Laurent Pinchart wrote: > Hi Hans, > > On Sun, Jan 16, 2022 at 10:43:25PM +0100, Hans de Goede wrote: >> Hi All, >> >> IIRC there was some discussion about $subject a while ago, >> esp. being pushed by the ChromeOS folks (IIRC). If you know what >> I'm talking about, please add relevant folks to the Cc. >> >> While doing some work on atomisp support I noticed that the >> ACPI device fwnode-s describing the sensors have an ACPI _PLD >> method, which is a standardized ACPI method to retreive an >> package (ACPI for struct) describing the location of things >> like USB ports; and in this case of the camera sensors. >> >> And upon checking the Surface Go DSDT the sensors there seem to >> have the _PLD bits to. And in both cases at least the following >> PLD field (bits 67-69) seems to contain valid and relevant info, >> quoting from the ACPI spec 6.2 version, page 329: >> >> """ >> Panel: Describes which panel surface of the system’s housing >> the device connection point resides on: >> 0 – Top >> 1 – Bottom >> 2 – Left >> 3 – Right >> 4 – Front >> 5 – Back >> 6 – Unknown >> """ >> >> This seems to be consistently set to 4 or 5 for the _PLD method >> of the sensor ACPI nodes which I checked. >> >> So rather then defining a new devicetree property for this and >> embedding that inside the ACPI tables, IMHO it would be best if >> the ChromeOS devices would use the standardized _PLD ACPI method >> for this too. > > I have no specific objection to this, given that the _PLD is > standardized. In your experience, is the rotation also populated > correctly ? That's important information too. That is a good question, so I just checked what the IPU3 does and it uses a field in the SSDB ACPI package for this. And I'm not sure that the _PLD is the right place for this, the _PLD is about how the ACPI object appears to the user, so that the operating-system can describe e.g. external connectors in a dialog box using this info. E.g. the _PLD also contains information about the color of the connector. And an upside-down sensor, does not look upside-down to the user when looking at it from the outside of the device. So based on this reading of the spec I don't expect the rotation field to contain what we are looking for (as is shown by the IPU3 driver using a SSDB field for this). > It we go in that direction, we should try to push OEMs to also populate > the vertical offset and horizontal offset fields, as I expect it to > become useful when multiple cameras are present in the same location. That is a good point. Note that my main reason for advertising using _PLD for the front/back info is that that is what is already being used in Windows laptops ACPI tables, so we need support for it regardless. Actually your rotation question made me wonder what we are doing for IPU3 here and we already have code parsing the _PLD in drivers/media/pci/intel/ipu3/cio2-bridge.c to set V4L2_FWNODE_ORIENTATION_FRONT / V4L2_FWNODE_ORIENTATION_BACK :) Since at least atomisp2 is going to need this to we probably need to factor this out into some shared helper. Regards, Hans
