The v4l2_fwnode_device_properties contains information about the
rotation. Use it if the ssdb data is inconclusive.
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
drivers/media/pci/intel/ipu-bridge.c | 30 +++++++++++++++++++-----------
1 file changed, 19 insertions(+), 11 deletions(-)
diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index 020aa52f590d66b6d333adc56ebfb9ab0561db51..6f436a8b4d23373af8a6668530333a827eca467a 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -236,37 +236,41 @@ static int ipu_bridge_read_acpi_buffer(struct acpi_device *adev, char *id,
}
static u32 ipu_bridge_parse_rotation(struct acpi_device *adev,
- struct ipu_sensor_ssdb *ssdb)
+ struct ipu_sensor_ssdb *ssdb,
+ struct v4l2_fwnode_device_properties *props)
{
switch (ssdb->degree) {
case IPU_SENSOR_ROTATION_NORMAL:
return 0;
case IPU_SENSOR_ROTATION_INVERTED:
return 180;
- default:
+ }
+
+ if (props->rotation == V4L2_FWNODE_PROPERTY_UNSET) {
dev_warn(ADEV_DEV(adev),
"Unknown rotation %d. Assume 0 degree rotation\n",
ssdb->degree);
return 0;
}
+
+ return props->rotation;
}
-static enum v4l2_fwnode_orientation ipu_bridge_parse_orientation(struct acpi_device *adev)
+static enum v4l2_fwnode_orientation
+ipu_bridge_parse_orientation(struct acpi_device *adev,
+ struct v4l2_fwnode_device_properties *props)
{
- struct v4l2_fwnode_device_properties props;
- int ret;
-
- ret = v4l2_fwnode_device_parse(ADEV_DEV(adev), &props);
- if (!ret || props.rotation == V4L2_FWNODE_PROPERTY_UNSET) {
+ if (props->orientation == V4L2_FWNODE_PROPERTY_UNSET) {
dev_warn(ADEV_DEV(adev), "Using default orientation\n");
return V4L2_FWNODE_ORIENTATION_EXTERNAL;
}
- return props.orientation;
+ return props->orientation;
}
int ipu_bridge_parse_ssdb(struct acpi_device *adev, struct ipu_sensor *sensor)
{
+ struct v4l2_fwnode_device_properties props;
struct ipu_sensor_ssdb ssdb = {};
int ret;
@@ -274,6 +278,10 @@ int ipu_bridge_parse_ssdb(struct acpi_device *adev, struct ipu_sensor *sensor)
if (ret)
return ret;
+ ret = v4l2_fwnode_device_parse(ADEV_DEV(adev), &props);
+ if (ret)
+ return ret;
+
if (ssdb.vcmtype > ARRAY_SIZE(ipu_vcm_types)) {
dev_warn(ADEV_DEV(adev), "Unknown VCM type %d\n", ssdb.vcmtype);
ssdb.vcmtype = 0;
@@ -287,8 +295,8 @@ int ipu_bridge_parse_ssdb(struct acpi_device *adev, struct ipu_sensor *sensor)
sensor->link = ssdb.link;
sensor->lanes = ssdb.lanes;
sensor->mclkspeed = ssdb.mclkspeed;
- sensor->rotation = ipu_bridge_parse_rotation(adev, &ssdb);
- sensor->orientation = ipu_bridge_parse_orientation(adev);
+ sensor->rotation = ipu_bridge_parse_rotation(adev, &ssdb, &props);
+ sensor->orientation = ipu_bridge_parse_orientation(adev, &props);
if (ssdb.vcmtype)
sensor->vcm_type = ipu_vcm_types[ssdb.vcmtype - 1];
--
2.50.0.rc0.642.g800a2b2222-googHi Ricardo,
On Thu, Jun 05, 2025 at 05:52:58PM +0000, Ricardo Ribalda wrote:
> 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?
_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°).
>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> drivers/media/pci/intel/ipu-bridge.c | 30 +++++++++++++++++++-----------
> 1 file changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> index 020aa52f590d66b6d333adc56ebfb9ab0561db51..6f436a8b4d23373af8a6668530333a827eca467a 100644
> --- a/drivers/media/pci/intel/ipu-bridge.c
> +++ b/drivers/media/pci/intel/ipu-bridge.c
> @@ -236,37 +236,41 @@ static int ipu_bridge_read_acpi_buffer(struct acpi_device *adev, char *id,
> }
>
> static u32 ipu_bridge_parse_rotation(struct acpi_device *adev,
> - struct ipu_sensor_ssdb *ssdb)
> + struct ipu_sensor_ssdb *ssdb,
> + struct v4l2_fwnode_device_properties *props)
> {
> switch (ssdb->degree) {
> case IPU_SENSOR_ROTATION_NORMAL:
> return 0;
> case IPU_SENSOR_ROTATION_INVERTED:
> return 180;
> - default:
> + }
> +
> + if (props->rotation == V4L2_FWNODE_PROPERTY_UNSET) {
> dev_warn(ADEV_DEV(adev),
> "Unknown rotation %d. Assume 0 degree rotation\n",
> ssdb->degree);
> return 0;
> }
> +
> + return props->rotation;
> }
>
> -static enum v4l2_fwnode_orientation ipu_bridge_parse_orientation(struct acpi_device *adev)
> +static enum v4l2_fwnode_orientation
> +ipu_bridge_parse_orientation(struct acpi_device *adev,
> + struct v4l2_fwnode_device_properties *props)
> {
> - struct v4l2_fwnode_device_properties props;
> - int ret;
> -
> - ret = v4l2_fwnode_device_parse(ADEV_DEV(adev), &props);
> - if (!ret || props.rotation == V4L2_FWNODE_PROPERTY_UNSET) {
> + if (props->orientation == V4L2_FWNODE_PROPERTY_UNSET) {
> dev_warn(ADEV_DEV(adev), "Using default orientation\n");
> return V4L2_FWNODE_ORIENTATION_EXTERNAL;
> }
>
> - return props.orientation;
> + return props->orientation;
> }
>
> int ipu_bridge_parse_ssdb(struct acpi_device *adev, struct ipu_sensor *sensor)
> {
> + struct v4l2_fwnode_device_properties props;
> struct ipu_sensor_ssdb ssdb = {};
> int ret;
>
> @@ -274,6 +278,10 @@ int ipu_bridge_parse_ssdb(struct acpi_device *adev, struct ipu_sensor *sensor)
> if (ret)
> return ret;
>
> + ret = v4l2_fwnode_device_parse(ADEV_DEV(adev), &props);
> + if (ret)
> + return ret;
> +
> if (ssdb.vcmtype > ARRAY_SIZE(ipu_vcm_types)) {
> dev_warn(ADEV_DEV(adev), "Unknown VCM type %d\n", ssdb.vcmtype);
> ssdb.vcmtype = 0;
> @@ -287,8 +295,8 @@ int ipu_bridge_parse_ssdb(struct acpi_device *adev, struct ipu_sensor *sensor)
> sensor->link = ssdb.link;
> sensor->lanes = ssdb.lanes;
> sensor->mclkspeed = ssdb.mclkspeed;
> - sensor->rotation = ipu_bridge_parse_rotation(adev, &ssdb);
> - sensor->orientation = ipu_bridge_parse_orientation(adev);
> + sensor->rotation = ipu_bridge_parse_rotation(adev, &ssdb, &props);
> + sensor->orientation = ipu_bridge_parse_orientation(adev, &props);
>
> if (ssdb.vcmtype)
> sensor->vcm_type = ipu_vcm_types[ssdb.vcmtype - 1];
>
--
Regards,
Sakari Ailus
Hi Sakari
Thanks for your review
On Mon, 7 Jul 2025 at 23:45, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
>
> Hi Ricardo,
>
> On Thu, Jun 05, 2025 at 05:52:58PM +0000, Ricardo Ribalda wrote:
> > 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?
>
> _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)
>
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> > drivers/media/pci/intel/ipu-bridge.c | 30 +++++++++++++++++++-----------
> > 1 file changed, 19 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> > index 020aa52f590d66b6d333adc56ebfb9ab0561db51..6f436a8b4d23373af8a6668530333a827eca467a 100644
> > --- a/drivers/media/pci/intel/ipu-bridge.c
> > +++ b/drivers/media/pci/intel/ipu-bridge.c
> > @@ -236,37 +236,41 @@ static int ipu_bridge_read_acpi_buffer(struct acpi_device *adev, char *id,
> > }
> >
> > static u32 ipu_bridge_parse_rotation(struct acpi_device *adev,
> > - struct ipu_sensor_ssdb *ssdb)
> > + struct ipu_sensor_ssdb *ssdb,
> > + struct v4l2_fwnode_device_properties *props)
> > {
> > switch (ssdb->degree) {
> > case IPU_SENSOR_ROTATION_NORMAL:
> > return 0;
> > case IPU_SENSOR_ROTATION_INVERTED:
> > return 180;
> > - default:
> > + }
> > +
> > + if (props->rotation == V4L2_FWNODE_PROPERTY_UNSET) {
> > dev_warn(ADEV_DEV(adev),
> > "Unknown rotation %d. Assume 0 degree rotation\n",
> > ssdb->degree);
> > return 0;
> > }
> > +
> > + return props->rotation;
> > }
> >
> > -static enum v4l2_fwnode_orientation ipu_bridge_parse_orientation(struct acpi_device *adev)
> > +static enum v4l2_fwnode_orientation
> > +ipu_bridge_parse_orientation(struct acpi_device *adev,
> > + struct v4l2_fwnode_device_properties *props)
> > {
> > - struct v4l2_fwnode_device_properties props;
> > - int ret;
> > -
> > - ret = v4l2_fwnode_device_parse(ADEV_DEV(adev), &props);
> > - if (!ret || props.rotation == V4L2_FWNODE_PROPERTY_UNSET) {
> > + if (props->orientation == V4L2_FWNODE_PROPERTY_UNSET) {
> > dev_warn(ADEV_DEV(adev), "Using default orientation\n");
> > return V4L2_FWNODE_ORIENTATION_EXTERNAL;
> > }
> >
> > - return props.orientation;
> > + return props->orientation;
> > }
> >
> > int ipu_bridge_parse_ssdb(struct acpi_device *adev, struct ipu_sensor *sensor)
> > {
> > + struct v4l2_fwnode_device_properties props;
> > struct ipu_sensor_ssdb ssdb = {};
> > int ret;
> >
> > @@ -274,6 +278,10 @@ int ipu_bridge_parse_ssdb(struct acpi_device *adev, struct ipu_sensor *sensor)
> > if (ret)
> > return ret;
> >
> > + ret = v4l2_fwnode_device_parse(ADEV_DEV(adev), &props);
> > + if (ret)
> > + return ret;
> > +
> > if (ssdb.vcmtype > ARRAY_SIZE(ipu_vcm_types)) {
> > dev_warn(ADEV_DEV(adev), "Unknown VCM type %d\n", ssdb.vcmtype);
> > ssdb.vcmtype = 0;
> > @@ -287,8 +295,8 @@ int ipu_bridge_parse_ssdb(struct acpi_device *adev, struct ipu_sensor *sensor)
> > sensor->link = ssdb.link;
> > sensor->lanes = ssdb.lanes;
> > sensor->mclkspeed = ssdb.mclkspeed;
> > - sensor->rotation = ipu_bridge_parse_rotation(adev, &ssdb);
> > - sensor->orientation = ipu_bridge_parse_orientation(adev);
> > + sensor->rotation = ipu_bridge_parse_rotation(adev, &ssdb, &props);
> > + sensor->orientation = ipu_bridge_parse_orientation(adev, &props);
> >
> > if (ssdb.vcmtype)
> > sensor->vcm_type = ipu_vcm_types[ssdb.vcmtype - 1];
> >
>
> --
> Regards,
>
> Sakari Ailus
--
Ricardo Ribalda
Hi Ricardo, On Tue, Jul 08, 2025 at 11:16:25AM +0200, Ricardo Ribalda wrote: > Hi Sakari > > Thanks for your review > > On Mon, 7 Jul 2025 at 23:45, Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > > > Hi Ricardo, > > > > On Thu, Jun 05, 2025 at 05:52:58PM +0000, Ricardo Ribalda wrote: > > > 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? > > > > > _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. -- Regards, Sakari Ailus
On Tue, 8 Jul 2025 at 11:22, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
>
> Hi Ricardo,
>
> On Tue, Jul 08, 2025 at 11:16:25AM +0200, Ricardo Ribalda wrote:
> > Hi Sakari
> >
> > Thanks for your review
> >
> > On Mon, 7 Jul 2025 at 23:45, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Ricardo,
> > >
> > > On Thu, Jun 05, 2025 at 05:52:58PM +0000, Ricardo Ribalda wrote:
> > > > 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?
> >
> > >
> > > _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);
return 0;
}
>
> --
> Regards,
>
> Sakari Ailus
--
Ricardo Ribalda
Hi Ricardo,
On Tue, Jul 08, 2025 at 02:09:28PM +0200, Ricardo Ribalda wrote:
> On Tue, 8 Jul 2025 at 11:22, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Ricardo,
> >
> > On Tue, Jul 08, 2025 at 11:16:25AM +0200, Ricardo Ribalda wrote:
> > > Hi Sakari
> > >
> > > Thanks for your review
> > >
> > > On Mon, 7 Jul 2025 at 23:45, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > > >
> > > > Hi Ricardo,
> > > >
> > > > On Thu, Jun 05, 2025 at 05:52:58PM +0000, Ricardo Ribalda wrote:
> > > > > 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?
> > >
> > > >
> > > > _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), ...);
?
> return 0;
> }
Looks good to me. Maybe something similar for orientation?
--
Regards,
Sakari Ailus
On Tue, 8 Jul 2025 at 14:21, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
>
> Hi Ricardo,
>
> On Tue, Jul 08, 2025 at 02:09:28PM +0200, Ricardo Ribalda wrote:
> > On Tue, 8 Jul 2025 at 11:22, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Ricardo,
> > >
> > > On Tue, Jul 08, 2025 at 11:16:25AM +0200, Ricardo Ribalda wrote:
> > > > Hi Sakari
> > > >
> > > > Thanks for your review
> > > >
> > > > On Mon, 7 Jul 2025 at 23:45, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > > > >
> > > > > Hi Ricardo,
> > > > >
> > > > > On Thu, Jun 05, 2025 at 05:52:58PM +0000, Ricardo Ribalda wrote:
> > > > > > 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?
> > > >
> > > > >
> > > > > _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), ...);
>
> ?
>
> > 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?
Regards!
>
> --
> Regards,
>
> Sakari Ailus
--
Ricardo Ribalda
Hi,
On 8-Jul-25 16:58, Ricardo Ribalda wrote:
> On Tue, 8 Jul 2025 at 14:21, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
>>
>> Hi Ricardo,
>>
>> On Tue, Jul 08, 2025 at 02:09:28PM +0200, Ricardo Ribalda wrote:
>>> On Tue, 8 Jul 2025 at 11:22, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
>>>>
>>>> Hi Ricardo,
>>>>
>>>> On Tue, Jul 08, 2025 at 11:16:25AM +0200, Ricardo Ribalda wrote:
>>>>> Hi Sakari
>>>>>
>>>>> Thanks for your review
>>>>>
>>>>> On Mon, 7 Jul 2025 at 23:45, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
>>>>>>
>>>>>> Hi Ricardo,
>>>>>>
>>>>>> On Thu, Jun 05, 2025 at 05:52:58PM +0000, Ricardo Ribalda wrote:
>>>>>>> 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?
>>>>>
>>>>>>
>>>>>> _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), ...);
>>
>> ?
>>
>>> 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
On Tue, Jul 08, 2025 at 04:58:25PM +0200, Ricardo Ribalda wrote:
> On Tue, 8 Jul 2025 at 14:21, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Ricardo,
> >
> > On Tue, Jul 08, 2025 at 02:09:28PM +0200, Ricardo Ribalda wrote:
> > > On Tue, 8 Jul 2025 at 11:22, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > > >
> > > > Hi Ricardo,
> > > >
> > > > On Tue, Jul 08, 2025 at 11:16:25AM +0200, Ricardo Ribalda wrote:
> > > > > Hi Sakari
> > > > >
> > > > > Thanks for your review
> > > > >
> > > > > On Mon, 7 Jul 2025 at 23:45, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > > > > >
> > > > > > Hi Ricardo,
> > > > > >
> > > > > > On Thu, Jun 05, 2025 at 05:52:58PM +0000, Ricardo Ribalda wrote:
> > > > > > > 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?
> > > > >
> > > > > >
> > > > > > _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), ...);
> >
> > ?
> >
> > > 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?
Ah, maybe SSDB has only rotation? At least it's less duplicated information
in different format, so that's a good thing. So this just applies to
rotation, it seems.
--
Sakari Ailus
© 2016 - 2025 Red Hat, Inc.