Hello, On Thu, Mar 30, 2023 at 06:27:25PM +0200, Jacopo Mondi wrote: > On Thu, Mar 30, 2023 at 11:50:15AM +0300, Sakari Ailus wrote: > > On Wed, Mar 29, 2023 at 08:21:06PM +0200, Jacopo Mondi wrote: > > > 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, I'd simplify it further: "Some systems have the camera sensor mounted upside down compared to its natural mounting rotation. In such cases, " More words can add clarity, but when they don't, excess words should be removed :-) > > > 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..." ? Sounds good, maybe with s/register/expose/. Or "drivers shall expose the information to userspace with the :ref:`V4L2_CID_CAMERA_SENSOR_ROTATION <v4l2-camera-sensor-rotation>` control." > > > I would also specify how drivers should initialize their flip controls > > > > > > "Mode-based sensor driver implementations that have any vertical or s/driver implementations/drivers/ (still chasing extraneous words) > > > 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 Agreed. I'd drop "Mode-based" though, I don't think that's relevant, other drivers may do something similar. > > > 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. I wouldn't for new drivers, no, and even for IMX258 and CCS, I'm tempted to not keep that backward compatibility. We need to assess whether or not it would cause breakages in practice, if the risk is very low, I'd favour consistency. This is especially the case for CCS. > > > > + > > > > +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! -- Regards, Laurent Pinchart
