Hi Laurent, On Wed, Apr 05, 2023 at 08:41:02AM +0300, Laurent Pinchart wrote: > 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 :-) Works for me. > > > > > 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. I'll change these for v3. > > > > > 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. I've actually already posted patches doing just this 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, Sakari Ailus
