Hi,

On 8-Jul-25 16:58, Ricardo Ribalda wrote:

On Tue, 8 Jul 2025 at 14:21, Sakari Ailus [off-list ref] wrote:

quoted

Hi Ricardo,

On Tue, Jul 08, 2025 at 02:09:28PM +0200, Ricardo Ribalda wrote:

quoted

On Tue, 8 Jul 2025 at 11:22, Sakari Ailus [off-list ref] wrote:

quoted

Hi Ricardo,

On Tue, Jul 08, 2025 at 11:16:25AM +0200, Ricardo Ribalda wrote:

quoted

Hi Sakari

Thanks for your review

On Mon, 7 Jul 2025 at 23:45, Sakari Ailus [off-list ref] wrote:

quoted

Hi Ricardo,

On Thu, Jun 05, 2025 at 05:52:58PM +0000, Ricardo Ribalda wrote:

quoted

The v4l2_fwnode_device_properties contains information about the
rotation. Use it if the ssdb data is inconclusive.

As SSDB and _PLD provide the same information, are they always aligned? Do
you have any experience on how is this actually in firmware?

Not really, in ChromeOS we are pretty lucky to control the firmware.

@HdG Do you have some experience/opinion here?

quoted

_PLD is standardised so it would seem reasonable to stick to that -- if it
exists. Another approach could be to pick the one that doesn't translate to
a sane default (0°).

I'd rather stick to the current prioritization unless there is a
strong argument against it. Otherwise there is a chance that we will
have regressions (outside CrOS)

My point was rather there are no such rules currently for rotation: only
SSDB was being used by the IPU bridge to obtain the rotation value,
similarly only _PLD is consulted when it comes to orientation.

So something like this:?

static u32 ipu_bridge_parse_rotation(struct acpi_device *adev,
                                     struct ipu_sensor_ssdb *ssdb,
                                     struct
v4l2_fwnode_device_properties *props)
{
        if (props->rotation != V4L2_FWNODE_PROPERTY_UNSET)
                return props->rotation;

        switch (ssdb->degree) {
        case IPU_SENSOR_ROTATION_NORMAL:
                return 0;
        case IPU_SENSOR_ROTATION_INVERTED:
                return 180;
        }

        dev_warn(ADEV_DEV(adev),
                 "Unknown rotation %d. Assume 0 degree rotation\n",
                 ssdb->degree);

Maybe:

        acpi_handle_warn(acpi_device_handle(adev), ...);

?

quoted

        return 0;
}

Looks good to me. Maybe something similar for orientation?

Do you mean using ssdb also for orientation or using acpi_handle_warn?


I cannot find anything related to orientation for SSDB
https://github.com/coreboot/coreboot/blob/main/src/drivers/intel/mipi_camera/chip.h#L150

Am I looking in the right place?

I believe that orientation is only available in the PLD,
so for orientation we can just use the value returned in
v4l2_fwnode_device_properties defaulting to front when
it is not set.

Otherwise I agree with what has been discussed in this
thread (for this patch) so far.

Regards,

Hans