Hi Jacopo, On Wed, Mar 29, 2023 at 08:21:06PM +0200, Jacopo Mondi wrote: > Hi Sakari > sorry, I missed this one! No problem. > > On Tue, Mar 28, 2023 at 05:52:48PM +0300, Sakari Ailus wrote: > > Document how rotation and orientation should be taken into account in > > writing camera sensor drivers. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> > > --- > > since v1: > > > > Use speclial double quotes around functions to use non-proportional font for > > them. > > > > Documentation/driver-api/media/camera-sensor.rst | 11 +++++++++++ > > .../userspace-api/media/v4l/ext-ctrls-camera.rst | 1 + > > 2 files changed, 12 insertions(+) > > > > diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst > > index c7d4891bd24e..2618a91b0d85 100644 > > --- a/Documentation/driver-api/media/camera-sensor.rst > > +++ b/Documentation/driver-api/media/camera-sensor.rst > > @@ -151,3 +151,14 @@ used to obtain device's power state after the power state transition: > > The function returns a non-zero value if it succeeded getting the power count or > > runtime PM was disabled, in either of which cases the driver may proceed to > > access the device. > > + > > +Rotation and orientation > > +------------------------ > > + > > +Some systems have been implemented so that the camera sensor has been mounted > > +upside down compared to its natural mounting rotation. In such a case, the > > +:ref:`V4L2_CID_CAMERA_SENSOR_ROTATION <v4l2-camera-sensor-rotation>` control > > +shall indicate the mounting rotation. > > I would put it in imperative form for driver developers > > "Some systems have been implemented so that the camera sensor has been mounted > upside down compared to its natural mounting rotation. In such a case, > drivers should register the :ref:`V4L2_CID_CAMERA_SENSOR_ROTATION > <v4l2-camera-sensor-rotation>` control to report the information to > userspace." That is in conditional, suggesting it isn't mandatory. Is that what you wanted to say? > > I would also specify how drivers should initialize their flip controls > > "Mode-based sensor driver implementations that have any vertical or > horizontal flips embedded in the register programming sequences should > initialize the V4L2_CID_HFLIP and V4L2_CID_VFLIP controls with the > values programmed by the register sequences. I'd use "shall" also here. I can't think of a reasonable exception as it breaks how this is meant to work for user space. > > Drivers which implement writable flip controls could automatically > compensate for the sensor's mounting rotation and shall reflect that in > the V4L2_CID_HFLIP and V4L2_CID_VFLIP controls initial and default values." Good point. For existing drivers (such as CCS or IMX258) this could help existing users but I'm not sure I'd do this for new drivers. Comments and opinions would be welcome from others, too. Cc Laurent, too. > > > + > > +Use ``v4l2_fwnode_device_parse()`` to obtain this information and > > +``v4l2_ctrl_new_fwnode_properties()`` to generate the appropriate controls. > > I would also mention the orientation control before introducing > v4l2_fwnode_device_parse. > > "Sensor drivers should also report their mounting orientation with the > :ref:`V4L2_CID_CAMERA_SENSOR_ORIENTATION <v4l2-camera-sensor-orientation>`. > > Both controls can be registered by using the ``v4l2_fwnode_device_parse()`` > function to obtain this information from the firmware interface and > ``v4l2_ctrl_new_fwnode_properties()`` to generate the appropriate controls." > > How does this work for you ? I'd just use "shall" here, too. This applies of course to new drivers only, many existing ones are missing these bits. -- Kind regards, Sakari Ailus
