Re: [PATCH v2 2/5] media: v4l: ctrls: Add a control for temperature

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Benjamin,

Thank you for the patch.

On Fri, Apr 15, 2022 at 01:18:42PM +0200, Benjamin Mugnier wrote:
> Add V4L2_CID_TEMPERATURE control to get temperature from sensor in
> celsius as a volatile and read-only control, and its documentation.
> Useful to monitor thermals from v4l controls for sensors that support
> this.
> 
> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> ---
>  Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst | 3 +++
>  drivers/media/v4l2-core/v4l2-ctrls-defs.c                  | 4 ++++
>  include/uapi/linux/v4l2-controls.h                         | 2 ++
>  3 files changed, 9 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> index 4c5061aa9cd4..26fa21f5c45a 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> @@ -661,3 +661,6 @@ enum v4l2_scene_mode -
>  .. [#f1]
>     This control may be changed to a menu control in the future, if more
>     options are required.
> +
> +``V4L2_CID_TEMPERATURE (integer)``
> +    The temperature of the sensor in celsius. This is a read-only control.

I've seen sensors where the temperature sensor has a 1/10th degree
precision. Should we standardize on that ? Anything more precise is
likely overkill.

There are also sensors with multiple temperature sensors. If there are
too many of them I suppose the temperature would be reported in embedded
data, but perhaps not always. How can we prepare for this ?

There are also a few details that I think should be documented. Is the
temperature always read on-demand when reading the control, or updated
periodically ? I would assume most drivers would implement the former,
which means no control notification events will be generated. This
should be documented. Furthermore, do drivers need to support reading
the temperature when the sensor isn't streaming ? If not, when should a
control read ioctl return, the last value, or an error ?

> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index 54ca4e6b820b..45ad3edd59e0 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -1042,6 +1042,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_UNIT_CELL_SIZE:		return "Unit Cell Size";
>  	case V4L2_CID_CAMERA_ORIENTATION:	return "Camera Orientation";
>  	case V4L2_CID_CAMERA_SENSOR_ROTATION:	return "Camera Sensor Rotation";
> +	case V4L2_CID_TEMPERATURE:		return "Temperature in °C";
>  
>  	/* FM Radio Modulator controls */
>  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> @@ -1597,6 +1598,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_RF_TUNER_PLL_LOCK:
>  		*flags |= V4L2_CTRL_FLAG_VOLATILE;
>  		break;
> +	case V4L2_CID_TEMPERATURE:
> +		*flags |= V4L2_CTRL_FLAG_READ_ONLY |
> +			  V4L2_CTRL_FLAG_VOLATILE;
>  	}
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_fill);
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index bb40129446d4..705b4043c2de 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -1008,6 +1008,8 @@ enum v4l2_auto_focus_range {
>  
>  #define V4L2_CID_CAMERA_SENSOR_ROTATION		(V4L2_CID_CAMERA_CLASS_BASE+35)
>  
> +#define V4L2_CID_TEMPERATURE			(V4L2_CID_CAMERA_CLASS_BASE+36)
> +
>  /* FM Modulator class control IDs */
>  
>  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)

-- 
Regards,

Laurent Pinchart