Hi Hans,
On 6/6/23 12:29, Hans Verkuil wrote:
> On 25/04/2023 11:45, Michael Riesch wrote:
>> A zoom lens group may consist of several lenses, and in a calibration
>> context it may be necessary to position the lenses individually. Add a
>> tuple of V4L2_CID_LENS_CALIB_ZOOMx_{ABSOLUTE,CURRENT,STATUS} controls
>> for each individual lens, where x = {1...5}.
>>
>> Signed-off-by: Michael Riesch <michael.riesch@xxxxxxxxxxxxxx>
>> ---
>> .../userspace-api/media/v4l/ext-ctrls-camera.rst | 30 ++++++++++++++++++++++
>> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 25 ++++++++++++++++++
>> include/uapi/linux/v4l2-controls.h | 18 +++++++++++++
>> 3 files changed, 73 insertions(+)
>>
>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> index 8b54a0f3a617..21391f076971 100644
>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> @@ -332,6 +332,36 @@ enum v4l2_auto_focus_range -
>> * - ``V4L2_LENS_CALIB_FAILED``
>> - Lens calibration procedure has failed.
>>
>> +``V4L2_CID_LENS_CALIB_ZOOM{1...5}_ABSOLUTE`` (integer)
>> + Set the absolute position of the individual lens of the zoom lens group.
>> + Most likely, this is done in a calibration context. The unit is
>> + driver-specific.
>> +
>> +``V4L2_CID_LENS_CALIB_ZOOM{1...5}_CURRENT`` (integer)
>> + The current absolute position of the individual lens of the zoom lens group.
>> + Most likely, this is done in a calibration context. The unit is
>> + driver-specific. This is a read-only control.
>> +
>> +``V4L2_CID_LENS_CALIB_ZOOM{1...5}_STATUS`` (bitmask)
>> + The current status of the individual lens of the zoom lens group.
>> + Most likely, this is done in a calibration context. The possible flags are
>> + described in the table below. This is a read-only control.
>
> Wouldn't it be better to have this as an array control? That way the number of
> lenses is set by the driver and can be easily read by userspace.
Are you referring to the _STATUS controls exclusively or is this a
suggestion for _CURRENT and _ABSOLUTE as well?
This would mean that the user space can read out e.g., the status of all
lenses in one batch, right? This would make sense to me.
I am not sure about setting all target positions at the same time ->
I'll have to think about that one a bit.
Best regards,
Michael
>
>> +
>> +.. tabularcolumns:: |p{6.8cm}|p{10.7cm}|
>> +
>> +.. flat-table::
>> + :header-rows: 0
>> + :stub-columns: 0
>> +
>> + * - ``V4L2_LENS_STATUS_IDLE``
>> + - Zoom lens is at rest.
>> + * - ``V4L2_LENS_STATUS_BUSY``
>> + - Zoom lens is moving.
>> + * - ``V4L2_LENS_STATUS_FAILED``
>> + - Zoom lens has failed to reach its target position. The driver will
>> + not transition from this state until another action is performed by an
>> + application.
>> +
>> ``V4L2_CID_IRIS_ABSOLUTE (integer)``
>> This control sets the camera's aperture to the specified value. The
>> unit is undefined. Larger values open the iris wider, smaller values
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> index faddfecba6d9..8a78cffcd3e8 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> @@ -1052,6 +1052,21 @@ const char *v4l2_ctrl_get_name(u32 id)
>> case V4L2_CID_ZOOM_SPEED: return "Zoom, Speed";
>> case V4L2_CID_LENS_CALIB_CONTROL: return "Lens Calibration, Control";
>> case V4L2_CID_LENS_CALIB_STATUS: return "Lens Calibration, Status";
>> + case V4L2_CID_LENS_CALIB_ZOOM1_ABSOLUTE: return "Zoom1, Absolute";
>> + case V4L2_CID_LENS_CALIB_ZOOM2_ABSOLUTE: return "Zoom2, Absolute";
>> + case V4L2_CID_LENS_CALIB_ZOOM3_ABSOLUTE: return "Zoom3, Absolute";
>> + case V4L2_CID_LENS_CALIB_ZOOM4_ABSOLUTE: return "Zoom4, Absolute";
>> + case V4L2_CID_LENS_CALIB_ZOOM5_ABSOLUTE: return "Zoom5, Absolute";
>> + case V4L2_CID_LENS_CALIB_ZOOM1_CURRENT: return "Zoom1, Current";
>> + case V4L2_CID_LENS_CALIB_ZOOM2_CURRENT: return "Zoom1, Current";
>> + case V4L2_CID_LENS_CALIB_ZOOM3_CURRENT: return "Zoom1, Current";
>> + case V4L2_CID_LENS_CALIB_ZOOM4_CURRENT: return "Zoom1, Current";
>> + case V4L2_CID_LENS_CALIB_ZOOM5_CURRENT: return "Zoom1, Current";
>
> You forget to update the number, it's all Zoom1 here.
>
>> + case V4L2_CID_LENS_CALIB_ZOOM1_STATUS: return "Zoom1, Status";
>> + case V4L2_CID_LENS_CALIB_ZOOM2_STATUS: return "Zoom2, Status";
>> + case V4L2_CID_LENS_CALIB_ZOOM3_STATUS: return "Zoom3, Status";
>> + case V4L2_CID_LENS_CALIB_ZOOM4_STATUS: return "Zoom4, Status";
>> + case V4L2_CID_LENS_CALIB_ZOOM5_STATUS: return "Zoom5, Status";
>>
>> /* FM Radio Modulator controls */
>> /* Keep the order of the 'case's the same as in v4l2-controls.h! */
>> @@ -1607,6 +1622,16 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>> case V4L2_CID_ZOOM_CURRENT:
>> case V4L2_CID_ZOOM_STATUS:
>> case V4L2_CID_LENS_CALIB_STATUS:
>> + case V4L2_CID_LENS_CALIB_ZOOM1_CURRENT:
>> + case V4L2_CID_LENS_CALIB_ZOOM2_CURRENT:
>> + case V4L2_CID_LENS_CALIB_ZOOM3_CURRENT:
>> + case V4L2_CID_LENS_CALIB_ZOOM4_CURRENT:
>> + case V4L2_CID_LENS_CALIB_ZOOM5_CURRENT:
>> + case V4L2_CID_LENS_CALIB_ZOOM1_STATUS:
>> + case V4L2_CID_LENS_CALIB_ZOOM2_STATUS:
>> + case V4L2_CID_LENS_CALIB_ZOOM3_STATUS:
>> + case V4L2_CID_LENS_CALIB_ZOOM4_STATUS:
>> + case V4L2_CID_LENS_CALIB_ZOOM5_STATUS:
>> *flags |= V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_VOLATILE;
>
> Same issue as for patch 3 w.r.t. VOLATILE.
>
>> break;
>> case V4L2_CID_FLASH_STROBE_STATUS:
>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>> index 24c0eb5f4d29..7c49c0ba23d4 100644
>> --- a/include/uapi/linux/v4l2-controls.h
>> +++ b/include/uapi/linux/v4l2-controls.h
>> @@ -1016,6 +1016,24 @@ enum v4l2_auto_focus_range {
>>
>> #define V4L2_CID_LENS_CALIB_STATUS (V4L2_CID_CAMERA_CLASS_BASE+44)
>>
>> +#define V4L2_CID_LENS_CALIB_ZOOM1_ABSOLUTE (V4L2_CID_CAMERA_CLASS_BASE+45)
>> +#define V4L2_CID_LENS_CALIB_ZOOM2_ABSOLUTE (V4L2_CID_CAMERA_CLASS_BASE+46)
>> +#define V4L2_CID_LENS_CALIB_ZOOM3_ABSOLUTE (V4L2_CID_CAMERA_CLASS_BASE+47)
>> +#define V4L2_CID_LENS_CALIB_ZOOM4_ABSOLUTE (V4L2_CID_CAMERA_CLASS_BASE+48)
>> +#define V4L2_CID_LENS_CALIB_ZOOM5_ABSOLUTE (V4L2_CID_CAMERA_CLASS_BASE+49)
>> +
>> +#define V4L2_CID_LENS_CALIB_ZOOM1_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+50)
>> +#define V4L2_CID_LENS_CALIB_ZOOM2_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+51)
>> +#define V4L2_CID_LENS_CALIB_ZOOM3_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+52)
>> +#define V4L2_CID_LENS_CALIB_ZOOM4_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+53)
>> +#define V4L2_CID_LENS_CALIB_ZOOM5_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+54)
>> +
>> +#define V4L2_CID_LENS_CALIB_ZOOM1_STATUS (V4L2_CID_CAMERA_CLASS_BASE+55)
>> +#define V4L2_CID_LENS_CALIB_ZOOM2_STATUS (V4L2_CID_CAMERA_CLASS_BASE+56)
>> +#define V4L2_CID_LENS_CALIB_ZOOM3_STATUS (V4L2_CID_CAMERA_CLASS_BASE+57)
>> +#define V4L2_CID_LENS_CALIB_ZOOM4_STATUS (V4L2_CID_CAMERA_CLASS_BASE+58)
>> +#define V4L2_CID_LENS_CALIB_ZOOM5_STATUS (V4L2_CID_CAMERA_CLASS_BASE+59)
>> +
>> /* FM Modulator class control IDs */
>>
>> #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900)
>>
>
> Disclaimer: I do not have enough domain knowledge to comment on if this is the
> right solution or not. I can only comment on the control framework specifics.
>
> Regards,
>
> Hans
