Hello On Thu, Mar 30, 2023 at 11:50:15AM +0300, Sakari Ailus wrote: > 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? > "drivers shall register..." ? > > > > 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. > Yes, I agree > > > > 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. > Agree on this as well > This applies of course to new drivers only, many existing ones are missing > these bits. > I know :( It's however worth it to encourage new drivers to do that! > -- > Kind regards, > > Sakari Ailus
