| Message ID | cover.1669381013.git.vkh@melexis.com |
|---|---|
| Headers | show |
| Series | media: i2c: mlx7502x ToF camera support | expand |
On 25/11/2022 14:34, Volodymyr Kharuk wrote: > Define Time of Flight class controls. > Also add most common TOF controls: > - phase sequence > - time integration > - frequency modulation > > Signed-off-by: Volodymyr Kharuk <vkh@melexis.com> > --- > include/uapi/linux/v4l2-controls.h | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h > index d27e255ed33b..a9ecfaa4252c 100644 > --- a/include/uapi/linux/v4l2-controls.h > +++ b/include/uapi/linux/v4l2-controls.h > @@ -68,6 +68,7 @@ > #define V4L2_CTRL_CLASS_DETECT 0x00a30000 /* Detection controls */ > #define V4L2_CTRL_CLASS_CODEC_STATELESS 0x00a40000 /* Stateless codecs controls */ > #define V4L2_CTRL_CLASS_COLORIMETRY 0x00a50000 /* Colorimetry controls */ > +#define V4L2_CTRL_CLASS_TOF 0x00a60000 /* Time of light camera controls */ light -> flight > > /* User-class control IDs */ > > @@ -2782,6 +2783,13 @@ struct v4l2_ctrl_vp9_compressed_hdr { > struct v4l2_vp9_mv_probs mv; > }; > > +#define V4L2_CID_TOF_CLASS_BASE (V4L2_CTRL_CLASS_TOF | 0x900) > +#define V4L2_CID_TOF_CLASS (V4L2_CTRL_CLASS_TOF | 1) > + > +#define V4L2_CID_TOF_PHASE_SEQ (V4L2_CID_TOF_CLASS_BASE + 0) > +#define V4L2_CID_TOF_FMOD (V4L2_CID_TOF_CLASS_BASE + 1) I'd go for _FREQ_MOD > +#define V4L2_CID_TOF_TINT (V4L2_CID_TOF_CLASS_BASE + 2) and _TIME_INTEGRATION Regards, Hans > + > /* MPEG-compression definitions kept for backwards compatibility */ > #ifndef __KERNEL__ > #define V4L2_CTRL_CLASS_MPEG V4L2_CTRL_CLASS_CODEC
On 25/11/2022 14:34, Volodymyr Kharuk wrote: > Define names, flags and types of TOF controls. *dims* is driver specific. > It also means, that it is not possible to use new_std for arrays. > > Signed-off-by: Volodymyr Kharuk <vkh@melexis.com> > --- > drivers/media/v4l2-core/v4l2-ctrls-defs.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c > index 564fedee2c88..1135d33c1baa 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c > @@ -1196,6 +1196,13 @@ const char *v4l2_ctrl_get_name(u32 id) > case V4L2_CID_COLORIMETRY_CLASS: return "Colorimetry Controls"; > case V4L2_CID_COLORIMETRY_HDR10_CLL_INFO: return "HDR10 Content Light Info"; > case V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY: return "HDR10 Mastering Display"; > + > + /* Time of light camera controls */ > + /* Keep the order of the 'case's the same as in v4l2-controls.h! */ > + case V4L2_CID_TOF_CLASS: return "Time of light Camera Controls"; light -> Flight > + case V4L2_CID_TOF_PHASE_SEQ: return "TOF phase sequence"; Capitalize, so: "TOF Phase Sequence" > + case V4L2_CID_TOF_FMOD: return "TOF frequency modulation"; "TOF Frequency Modulation" > + case V4L2_CID_TOF_TINT: return "TOF time integration"; "TOF Time Integration" > default: > return NULL; > } > @@ -1403,6 +1410,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, > case V4L2_CID_DETECT_CLASS: > case V4L2_CID_CODEC_STATELESS_CLASS: > case V4L2_CID_COLORIMETRY_CLASS: > + case V4L2_CID_TOF_CLASS: > *type = V4L2_CTRL_TYPE_CTRL_CLASS; > /* You can neither read nor write these */ > *flags |= V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY; > @@ -1541,6 +1549,18 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, > case V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY: > *type = V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY; > break; > + case V4L2_CID_TOF_PHASE_SEQ: > + *type = V4L2_CTRL_TYPE_U16; > + *flags |= V4L2_CTRL_FLAG_DYNAMIC_ARRAY; > + break; > + case V4L2_CID_TOF_FMOD: > + *type = V4L2_CTRL_TYPE_U8; > + *flags |= V4L2_CTRL_FLAG_DYNAMIC_ARRAY; > + break; > + case V4L2_CID_TOF_TINT: > + *type = V4L2_CTRL_TYPE_U16; > + *flags |= V4L2_CTRL_FLAG_DYNAMIC_ARRAY; > + break; > default: > *type = V4L2_CTRL_TYPE_INTEGER; > break; Regards, Hans
On 25/11/2022 14:34, Volodymyr Kharuk wrote: > Add description about V4L2_CID_TOF_PHASE_SEQ, V4L2_CID_TOF_FMOD > and V4L2_CID_TOF_TINT. > Also updated MAINTAINERS with new ext-ctrls-tof file. > > Signed-off-by: Volodymyr Kharuk <vkh@melexis.com> > --- > .../userspace-api/media/v4l/common.rst | 1 + > .../userspace-api/media/v4l/ext-ctrls-tof.rst | 35 +++++++++++++++++++ > MAINTAINERS | 7 ++++ > 3 files changed, 43 insertions(+) > create mode 100644 Documentation/userspace-api/media/v4l/ext-ctrls-tof.rst > > diff --git a/Documentation/userspace-api/media/v4l/common.rst b/Documentation/userspace-api/media/v4l/common.rst > index ea0435182e44..1ea79e453066 100644 > --- a/Documentation/userspace-api/media/v4l/common.rst > +++ b/Documentation/userspace-api/media/v4l/common.rst > @@ -52,6 +52,7 @@ applicable to all devices. > ext-ctrls-fm-rx > ext-ctrls-detect > ext-ctrls-colorimetry > + ext-ctrls-tof > fourcc > format > planar-apis > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-tof.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-tof.rst > new file mode 100644 > index 000000000000..8902cc7cd47b > --- /dev/null > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-tof.rst > @@ -0,0 +1,35 @@ > +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later > + > +.. _tof-controls: > + > +*************************************** > +Time of Flight Camera Control Reference > +*************************************** > + > +The Time of Flight class includes controls for digital features > +of TOF camera. You might want to extend this description a bit and give more info about how they work. Perhaps a link to wikipedia or something might help too. > + > +.. _tof-control-id: > + > +Time of Flight Camera Control IDs > +================================= > + > +``V4L2_CID_TOF_CLASS (class)`` > + The TOF class descriptor. Calling :ref:`VIDIOC_QUERYCTRL` for > + this control will return a description of this control class. > + > +``V4L2_CID_TOF_PHASE_SEQ (dynamic array u16)`` > + Change the shift between illumination and sampling for each phase > + in degrees. A distance/confidence picture is obtained by merging > + 3..8 captures of the same scene using different phase shifts(some Space before ( > + TOF sensors use different frequency modulation). Either: use -> use a Or: modulation -> modulations It's not clear right now whether "frequency modulation" is meant to be singular or plural. > + > + The maximum array size is driver specific. > + > +``V4L2_CID_TOF_FMOD (dynamic array u8)`` > + The control sets the modulation frequency(in Mhz) per each phase. Space before ( per each phase -> for each phase > + The maximum array size is driver specific. What does the maximum array size signify? The number of phases? It's not clear from the spec (and I have to admit I know very little about TOF sensors). > + > +``V4L2_CID_TOF_TINT (dynamic array u16)`` > + The control sets the integration time(in us) per each phase. Add space before ( per each phase -> for each phase > + The maximum array size is driver specific. > diff --git a/MAINTAINERS b/MAINTAINERS > index aa1974054fce..a2bc2ce53056 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -13111,6 +13111,13 @@ S: Supported > W: http://www.melexis.com > F: drivers/iio/temperature/mlx90632.c > > +MELEXIS MLX7502X DRIVER > +M: Volodymyr Kharuk <vkh@melexis.com> > +L: linux-media@vger.kernel.org > +S: Supported > +W: http://www.melexis.com > +F: Documentation/userspace-api/media/v4l/ext-ctrls-tof.rst > + > MELFAS MIP4 TOUCHSCREEN DRIVER > M: Sangwon Jee <jeesw@melfas.com> > S: Supported Regards, Hans
On 25/11/2022 14:34, Volodymyr Kharuk wrote: > For V4L2_CID_TOF_TINT, which is dynamic array, it is required to use > __v4l2_ctrl_modify_range. So the idea is to use type_ops instead of u64 > from union. It will allow to work with any type. > > Signed-off-by: Volodymyr Kharuk <vkh@melexis.com> > --- > drivers/media/v4l2-core/v4l2-ctrls-api.c | 15 +++------------ > 1 file changed, 3 insertions(+), 12 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c > index d0a3aa3806fb..09735644a2f1 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c > @@ -942,8 +942,6 @@ int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl, > case V4L2_CTRL_TYPE_U8: > case V4L2_CTRL_TYPE_U16: > case V4L2_CTRL_TYPE_U32: > - if (ctrl->is_array) > - return -EINVAL; > ret = check_range(ctrl->type, min, max, step, def); > if (ret) > return ret; > @@ -960,17 +958,10 @@ int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl, > ctrl->default_value = def; > } > cur_to_new(ctrl); > - if (validate_new(ctrl, ctrl->p_new)) { > - if (ctrl->type == V4L2_CTRL_TYPE_INTEGER64) > - *ctrl->p_new.p_s64 = def; > - else > - *ctrl->p_new.p_s32 = def; > - } > + if (validate_new(ctrl, ctrl->p_new)) > + ctrl->type_ops->init(ctrl, 0, ctrl->p_new); Hmm, this sets *all* elements of the array to the default_value, not just the elements whose value is out of range. Is that what you want? Should this perhaps depend on the type of control? I.e. in some cases it might make sense to do this, in other cases it makes more sense to only adjust the elements that are out of range. How does that work for this TINT control? Regards, Hans > > - if (ctrl->type == V4L2_CTRL_TYPE_INTEGER64) > - value_changed = *ctrl->p_new.p_s64 != *ctrl->p_cur.p_s64; > - else > - value_changed = *ctrl->p_new.p_s32 != *ctrl->p_cur.p_s32; > + value_changed = !ctrl->type_ops->equal(ctrl, ctrl->p_cur, ctrl->p_new); > if (value_changed) > ret = set_ctrl(NULL, ctrl, V4L2_EVENT_CTRL_CH_RANGE); > else if (range_changed)
On 25/11/2022 14:34, Volodymyr Kharuk wrote: > Define user controls for mlx7502x driver, add its documentation and > update MAINTAINERS > > Signed-off-by: Volodymyr Kharuk <vkh@melexis.com> > --- > .../userspace-api/media/drivers/index.rst | 1 + > .../userspace-api/media/drivers/mlx7502x.rst | 28 +++++++++++++++++++ > MAINTAINERS | 2 ++ > include/uapi/linux/mlx7502x.h | 20 +++++++++++++ > 4 files changed, 51 insertions(+) > create mode 100644 Documentation/userspace-api/media/drivers/mlx7502x.rst > create mode 100644 include/uapi/linux/mlx7502x.h > > diff --git a/Documentation/userspace-api/media/drivers/index.rst b/Documentation/userspace-api/media/drivers/index.rst > index 32f82aed47d9..f49e1b64c256 100644 > --- a/Documentation/userspace-api/media/drivers/index.rst > +++ b/Documentation/userspace-api/media/drivers/index.rst > @@ -37,5 +37,6 @@ For more details see the file COPYING in the source distribution of Linux. > imx-uapi > max2175 > meye-uapi > + mlx7502x > omap3isp-uapi > uvcvideo > diff --git a/Documentation/userspace-api/media/drivers/mlx7502x.rst b/Documentation/userspace-api/media/drivers/mlx7502x.rst > new file mode 100644 > index 000000000000..6f4874ec010d > --- /dev/null > +++ b/Documentation/userspace-api/media/drivers/mlx7502x.rst > @@ -0,0 +1,28 @@ > +.. SPDX-License-Identifier: GPL-2.0 > + > +Melexis mlx7502x ToF camera sensor driver > +========================================= > + > +The mlx7502x driver implements the following driver-specific controls: > + > +``V4L2_CID_MLX7502X_OUTPUT_MODE (menu)`` > +---------------------------------------- > + The sensor has two taps, which gather reflected light: A and B. > + The control sets the way data should be put in a buffer. The most > + common output mode is A-B which provides the best sunlight robustness. > + > +.. flat-table:: > + :header-rows: 0 > + :stub-columns: 0 > + :widths: 1 4 > + > + * - ``(0)`` > + - A minus B > + * - ``(1)`` > + - A plus B > + * - ``(2)`` > + - only A > + * - ``(3)`` > + - only B > + * - ``(4)`` > + - A and B (this config will change PAD format) > diff --git a/MAINTAINERS b/MAINTAINERS > index a2bc2ce53056..0a6dda8da6bc 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -13116,7 +13116,9 @@ M: Volodymyr Kharuk <vkh@melexis.com> > L: linux-media@vger.kernel.org > S: Supported > W: http://www.melexis.com > +F: Documentation/userspace-api/media/drivers/mlx7502x.rst > F: Documentation/userspace-api/media/v4l/ext-ctrls-tof.rst > +F: include/uapi/linux/mlx7502x.h > > MELFAS MIP4 TOUCHSCREEN DRIVER > M: Sangwon Jee <jeesw@melfas.com> > diff --git a/include/uapi/linux/mlx7502x.h b/include/uapi/linux/mlx7502x.h > new file mode 100644 > index 000000000000..68014f550ed2 > --- /dev/null > +++ b/include/uapi/linux/mlx7502x.h > @@ -0,0 +1,20 @@ > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ > +/* > + * Melexis 7502x ToF cameras driver. > + * > + * Copyright (C) 2021 Melexis N.V. > + * > + */ > + > +#ifndef __UAPI_MLX7502X_H_ > +#define __UAPI_MLX7502X_H_ > + > +#include <linux/v4l2-controls.h> > + > +/* > + * this is related to the taps in ToF cameras, > + * usually A minus B is the best option > + */ > +#define V4L2_CID_MLX7502X_OUTPUT_MODE (V4L2_CID_USER_MLX7502X_BASE + 0) You need to add an enum with the mode settings. E.g.: enum v4l2_mlx7502x_output_mode { V4L2_MLX7502X_OUTPUT_MODE_A_MINUS_B = 0, ... }; And you can use those enum defines in the documentation. Regards, Hans > + > +#endif /* __UAPI_MLX7502X_H_ */
Hi Hans, Thanks for your review, On Fri, Nov 25, 2022 at 03:20:46PM +0100, Hans Verkuil wrote: > On 25/11/2022 14:34, Volodymyr Kharuk wrote: > > Define Time of Flight class controls. > > Also add most common TOF controls: > > - phase sequence > > - time integration > > - frequency modulation > > > > Signed-off-by: Volodymyr Kharuk <vkh@melexis.com> > > --- > > include/uapi/linux/v4l2-controls.h | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h > > index d27e255ed33b..a9ecfaa4252c 100644 > > --- a/include/uapi/linux/v4l2-controls.h > > +++ b/include/uapi/linux/v4l2-controls.h > > @@ -68,6 +68,7 @@ > > #define V4L2_CTRL_CLASS_DETECT 0x00a30000 /* Detection controls */ > > #define V4L2_CTRL_CLASS_CODEC_STATELESS 0x00a40000 /* Stateless codecs controls */ > > #define V4L2_CTRL_CLASS_COLORIMETRY 0x00a50000 /* Colorimetry controls */ > > +#define V4L2_CTRL_CLASS_TOF 0x00a60000 /* Time of light camera controls */ > > light -> flight oh, indeed. Will fix. > > > > > /* User-class control IDs */ > > > > @@ -2782,6 +2783,13 @@ struct v4l2_ctrl_vp9_compressed_hdr { > > struct v4l2_vp9_mv_probs mv; > > }; > > > > +#define V4L2_CID_TOF_CLASS_BASE (V4L2_CTRL_CLASS_TOF | 0x900) > > +#define V4L2_CID_TOF_CLASS (V4L2_CTRL_CLASS_TOF | 1) > > + > > +#define V4L2_CID_TOF_PHASE_SEQ (V4L2_CID_TOF_CLASS_BASE + 0) > > +#define V4L2_CID_TOF_FMOD (V4L2_CID_TOF_CLASS_BASE + 1) > > I'd go for _FREQ_MOD Ok. Will fix. > > > +#define V4L2_CID_TOF_TINT (V4L2_CID_TOF_CLASS_BASE + 2) > > and _TIME_INTEGRATION Ok. Will fix. > > Regards, > > Hans > > > + > > /* MPEG-compression definitions kept for backwards compatibility */ > > #ifndef __KERNEL__ > > #define V4L2_CTRL_CLASS_MPEG V4L2_CTRL_CLASS_CODEC >
Thanks for your review. I'll fix your remarks in next version. On Fri, Nov 25, 2022 at 03:22:16PM +0100, Hans Verkuil wrote: > On 25/11/2022 14:34, Volodymyr Kharuk wrote: > > Define names, flags and types of TOF controls. *dims* is driver specific. > > It also means, that it is not possible to use new_std for arrays. > > > > Signed-off-by: Volodymyr Kharuk <vkh@melexis.com> > > --- > > drivers/media/v4l2-core/v4l2-ctrls-defs.c | 20 ++++++++++++++++++++ > > 1 file changed, 20 insertions(+) > > > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c > > index 564fedee2c88..1135d33c1baa 100644 > > --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c > > +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c > > @@ -1196,6 +1196,13 @@ const char *v4l2_ctrl_get_name(u32 id) > > case V4L2_CID_COLORIMETRY_CLASS: return "Colorimetry Controls"; > > case V4L2_CID_COLORIMETRY_HDR10_CLL_INFO: return "HDR10 Content Light Info"; > > case V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY: return "HDR10 Mastering Display"; > > + > > + /* Time of light camera controls */ > > + /* Keep the order of the 'case's the same as in v4l2-controls.h! */ > > + case V4L2_CID_TOF_CLASS: return "Time of light Camera Controls"; > > light -> Flight > > > + case V4L2_CID_TOF_PHASE_SEQ: return "TOF phase sequence"; > > Capitalize, so: "TOF Phase Sequence" > > > + case V4L2_CID_TOF_FMOD: return "TOF frequency modulation"; > > "TOF Frequency Modulation" > > > + case V4L2_CID_TOF_TINT: return "TOF time integration"; > > "TOF Time Integration" > > > default: > > return NULL; > > } > > @@ -1403,6 +1410,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, > > case V4L2_CID_DETECT_CLASS: > > case V4L2_CID_CODEC_STATELESS_CLASS: > > case V4L2_CID_COLORIMETRY_CLASS: > > + case V4L2_CID_TOF_CLASS: > > *type = V4L2_CTRL_TYPE_CTRL_CLASS; > > /* You can neither read nor write these */ > > *flags |= V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY; > > @@ -1541,6 +1549,18 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, > > case V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY: > > *type = V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY; > > break; > > + case V4L2_CID_TOF_PHASE_SEQ: > > + *type = V4L2_CTRL_TYPE_U16; > > + *flags |= V4L2_CTRL_FLAG_DYNAMIC_ARRAY; > > + break; > > + case V4L2_CID_TOF_FMOD: > > + *type = V4L2_CTRL_TYPE_U8; > > + *flags |= V4L2_CTRL_FLAG_DYNAMIC_ARRAY; > > + break; > > + case V4L2_CID_TOF_TINT: > > + *type = V4L2_CTRL_TYPE_U16; > > + *flags |= V4L2_CTRL_FLAG_DYNAMIC_ARRAY; > > + break; > > default: > > *type = V4L2_CTRL_TYPE_INTEGER; > > break; > > Regards, > > Hans
On Fri, Nov 25, 2022 at 03:39:16PM +0100, Hans Verkuil wrote: > On 25/11/2022 14:34, Volodymyr Kharuk wrote: > > Define user controls for mlx7502x driver, add its documentation and > > update MAINTAINERS > > > > Signed-off-by: Volodymyr Kharuk <vkh@melexis.com> > > --- > > .../userspace-api/media/drivers/index.rst | 1 + > > .../userspace-api/media/drivers/mlx7502x.rst | 28 +++++++++++++++++++ > > MAINTAINERS | 2 ++ > > include/uapi/linux/mlx7502x.h | 20 +++++++++++++ > > 4 files changed, 51 insertions(+) > > create mode 100644 Documentation/userspace-api/media/drivers/mlx7502x.rst > > create mode 100644 include/uapi/linux/mlx7502x.h > > > > diff --git a/Documentation/userspace-api/media/drivers/index.rst b/Documentation/userspace-api/media/drivers/index.rst > > index 32f82aed47d9..f49e1b64c256 100644 > > --- a/Documentation/userspace-api/media/drivers/index.rst > > +++ b/Documentation/userspace-api/media/drivers/index.rst > > @@ -37,5 +37,6 @@ For more details see the file COPYING in the source distribution of Linux. > > imx-uapi > > max2175 > > meye-uapi > > + mlx7502x > > omap3isp-uapi > > uvcvideo > > diff --git a/Documentation/userspace-api/media/drivers/mlx7502x.rst b/Documentation/userspace-api/media/drivers/mlx7502x.rst > > new file mode 100644 > > index 000000000000..6f4874ec010d > > --- /dev/null > > +++ b/Documentation/userspace-api/media/drivers/mlx7502x.rst > > @@ -0,0 +1,28 @@ > > +.. SPDX-License-Identifier: GPL-2.0 > > + > > +Melexis mlx7502x ToF camera sensor driver > > +========================================= > > + > > +The mlx7502x driver implements the following driver-specific controls: > > + > > +``V4L2_CID_MLX7502X_OUTPUT_MODE (menu)`` > > +---------------------------------------- > > + The sensor has two taps, which gather reflected light: A and B. > > + The control sets the way data should be put in a buffer. The most > > + common output mode is A-B which provides the best sunlight robustness. > > + > > +.. flat-table:: > > + :header-rows: 0 > > + :stub-columns: 0 > > + :widths: 1 4 > > + > > + * - ``(0)`` > > + - A minus B > > + * - ``(1)`` > > + - A plus B > > + * - ``(2)`` > > + - only A > > + * - ``(3)`` > > + - only B > > + * - ``(4)`` > > + - A and B (this config will change PAD format) > > diff --git a/MAINTAINERS b/MAINTAINERS > > index a2bc2ce53056..0a6dda8da6bc 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -13116,7 +13116,9 @@ M: Volodymyr Kharuk <vkh@melexis.com> > > L: linux-media@vger.kernel.org > > S: Supported > > W: http://www.melexis.com > > +F: Documentation/userspace-api/media/drivers/mlx7502x.rst > > F: Documentation/userspace-api/media/v4l/ext-ctrls-tof.rst > > +F: include/uapi/linux/mlx7502x.h > > > > MELFAS MIP4 TOUCHSCREEN DRIVER > > M: Sangwon Jee <jeesw@melfas.com> > > diff --git a/include/uapi/linux/mlx7502x.h b/include/uapi/linux/mlx7502x.h > > new file mode 100644 > > index 000000000000..68014f550ed2 > > --- /dev/null > > +++ b/include/uapi/linux/mlx7502x.h > > @@ -0,0 +1,20 @@ > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ > > +/* > > + * Melexis 7502x ToF cameras driver. > > + * > > + * Copyright (C) 2021 Melexis N.V. > > + * > > + */ > > + > > +#ifndef __UAPI_MLX7502X_H_ > > +#define __UAPI_MLX7502X_H_ > > + > > +#include <linux/v4l2-controls.h> > > + > > +/* > > + * this is related to the taps in ToF cameras, > > + * usually A minus B is the best option > > + */ > > +#define V4L2_CID_MLX7502X_OUTPUT_MODE (V4L2_CID_USER_MLX7502X_BASE + 0) > > You need to add an enum with the mode settings. E.g.: > > enum v4l2_mlx7502x_output_mode { > V4L2_MLX7502X_OUTPUT_MODE_A_MINUS_B = 0, > ... > }; > > And you can use those enum defines in the documentation. Ok, thanks. That is is interesting. Will fix in next version. > > Regards, > > Hans > > > + > > +#endif /* __UAPI_MLX7502X_H_ */ >
Thanks for review. On Fri, Nov 25, 2022 at 03:28:30PM +0100, Hans Verkuil wrote: > On 25/11/2022 14:34, Volodymyr Kharuk wrote: > > Add description about V4L2_CID_TOF_PHASE_SEQ, V4L2_CID_TOF_FMOD > > and V4L2_CID_TOF_TINT. > > Also updated MAINTAINERS with new ext-ctrls-tof file. > > > > Signed-off-by: Volodymyr Kharuk <vkh@melexis.com> > > --- > > .../userspace-api/media/v4l/common.rst | 1 + > > .../userspace-api/media/v4l/ext-ctrls-tof.rst | 35 +++++++++++++++++++ > > MAINTAINERS | 7 ++++ > > 3 files changed, 43 insertions(+) > > create mode 100644 Documentation/userspace-api/media/v4l/ext-ctrls-tof.rst > > > > diff --git a/Documentation/userspace-api/media/v4l/common.rst b/Documentation/userspace-api/media/v4l/common.rst > > index ea0435182e44..1ea79e453066 100644 > > --- a/Documentation/userspace-api/media/v4l/common.rst > > +++ b/Documentation/userspace-api/media/v4l/common.rst > > @@ -52,6 +52,7 @@ applicable to all devices. > > ext-ctrls-fm-rx > > ext-ctrls-detect > > ext-ctrls-colorimetry > > + ext-ctrls-tof > > fourcc > > format > > planar-apis > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-tof.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-tof.rst > > new file mode 100644 > > index 000000000000..8902cc7cd47b > > --- /dev/null > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-tof.rst > > @@ -0,0 +1,35 @@ > > +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later > > + > > +.. _tof-controls: > > + > > +*************************************** > > +Time of Flight Camera Control Reference > > +*************************************** > > + > > +The Time of Flight class includes controls for digital features > > +of TOF camera. > > You might want to extend this description a bit and give more info > about how they work. Perhaps a link to wikipedia or something > might help too. I was not sure what to add here. Ok, I will update. > > > + > > +.. _tof-control-id: > > + > > +Time of Flight Camera Control IDs > > +================================= > > + > > +``V4L2_CID_TOF_CLASS (class)`` > > + The TOF class descriptor. Calling :ref:`VIDIOC_QUERYCTRL` for > > + this control will return a description of this control class. > > + > > +``V4L2_CID_TOF_PHASE_SEQ (dynamic array u16)`` > > + Change the shift between illumination and sampling for each phase > > + in degrees. A distance/confidence picture is obtained by merging > > + 3..8 captures of the same scene using different phase shifts(some > > Space before ( > > > + TOF sensors use different frequency modulation). > > Either: use -> use a > Or: modulation -> modulations > > It's not clear right now whether "frequency modulation" is meant to be singular > or plural. > > > + > > + The maximum array size is driver specific. > > + > > +``V4L2_CID_TOF_FMOD (dynamic array u8)`` > > + The control sets the modulation frequency(in Mhz) per each phase. > > Space before ( > > per each phase -> for each phase > > > + The maximum array size is driver specific. > > What does the maximum array size signify? The number of phases? > It's not clear from the spec (and I have to admit I know very little > about TOF sensors). yes, array size defines the number of phases. But the maximum number of phases can be different and depend on the sensor. I'll update the doc. > > > + > > +``V4L2_CID_TOF_TINT (dynamic array u16)`` > > + The control sets the integration time(in us) per each phase. > > Add space before ( > > per each phase -> for each phase > > > + The maximum array size is driver specific. > > diff --git a/MAINTAINERS b/MAINTAINERS > > index aa1974054fce..a2bc2ce53056 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -13111,6 +13111,13 @@ S: Supported > > W: http://www.melexis.com > > F: drivers/iio/temperature/mlx90632.c > > > > +MELEXIS MLX7502X DRIVER > > +M: Volodymyr Kharuk <vkh@melexis.com> > > +L: linux-media@vger.kernel.org > > +S: Supported > > +W: http://www.melexis.com > > +F: Documentation/userspace-api/media/v4l/ext-ctrls-tof.rst > > + > > MELFAS MIP4 TOUCHSCREEN DRIVER > > M: Sangwon Jee <jeesw@melfas.com> > > S: Supported > > Regards, > > Hans
Hi Hans, On Fri, Nov 25, 2022 at 03:35:34PM +0100, Hans Verkuil wrote: > On 25/11/2022 14:34, Volodymyr Kharuk wrote: > > For V4L2_CID_TOF_TINT, which is dynamic array, it is required to use > > __v4l2_ctrl_modify_range. So the idea is to use type_ops instead of u64 > > from union. It will allow to work with any type. > > > > Signed-off-by: Volodymyr Kharuk <vkh@melexis.com> > > --- > > drivers/media/v4l2-core/v4l2-ctrls-api.c | 15 +++------------ > > 1 file changed, 3 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c > > index d0a3aa3806fb..09735644a2f1 100644 > > --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c > > +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c > > @@ -942,8 +942,6 @@ int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl, > > case V4L2_CTRL_TYPE_U8: > > case V4L2_CTRL_TYPE_U16: > > case V4L2_CTRL_TYPE_U32: > > - if (ctrl->is_array) > > - return -EINVAL; > > ret = check_range(ctrl->type, min, max, step, def); > > if (ret) > > return ret; > > @@ -960,17 +958,10 @@ int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl, > > ctrl->default_value = def; > > } > > cur_to_new(ctrl); > > - if (validate_new(ctrl, ctrl->p_new)) { > > - if (ctrl->type == V4L2_CTRL_TYPE_INTEGER64) > > - *ctrl->p_new.p_s64 = def; > > - else > > - *ctrl->p_new.p_s32 = def; > > - } > > + if (validate_new(ctrl, ctrl->p_new)) > > + ctrl->type_ops->init(ctrl, 0, ctrl->p_new); > > Hmm, this sets *all* elements of the array to the default_value, not > just the elements whose value is out of range. > > Is that what you want? Should this perhaps depend on the type of > control? I.e. in some cases it might make sense to do this, in other > cases it makes more sense to only adjust the elements that are out > of range. > > How does that work for this TINT control? Actually for types like INTEGER/U8/U16/U32/BOOLEAN/BUTTON/BITMASK, the function validate_new will return zero always as well as they fix the range on the fly. So that is ok for mlx7502x sensors. For types like compound/string/menu indeed, it will sets all elements of array to default array. To fix it I propose to fix it by using the functions std_validate_elem to check per element and then set the default manually. But then it means, that __v4l2_ctrl_modify_range will work only for standart v4l2 types, and not if driver use their own implementation. Is it fine for you? What do you think? > > Regards, > > Hans > > > > > - if (ctrl->type == V4L2_CTRL_TYPE_INTEGER64) > > - value_changed = *ctrl->p_new.p_s64 != *ctrl->p_cur.p_s64; > > - else > > - value_changed = *ctrl->p_new.p_s32 != *ctrl->p_cur.p_s32; > > + value_changed = !ctrl->type_ops->equal(ctrl, ctrl->p_cur, ctrl->p_new); > > if (value_changed) > > ret = set_ctrl(NULL, ctrl, V4L2_EVENT_CTRL_CH_RANGE); > > else if (range_changed) >
On Thu, Dec 01, 2022 at 05:44:52PM +0200, Volodymyr Kharuk wrote: > Hi Hans, > > On Fri, Nov 25, 2022 at 03:35:34PM +0100, Hans Verkuil wrote: > > On 25/11/2022 14:34, Volodymyr Kharuk wrote: > > > For V4L2_CID_TOF_TINT, which is dynamic array, it is required to use > > > __v4l2_ctrl_modify_range. So the idea is to use type_ops instead of u64 > > > from union. It will allow to work with any type. > > > > > > Signed-off-by: Volodymyr Kharuk <vkh@melexis.com> > > > --- > > > drivers/media/v4l2-core/v4l2-ctrls-api.c | 15 +++------------ > > > 1 file changed, 3 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c > > > index d0a3aa3806fb..09735644a2f1 100644 > > > --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c > > > +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c > > > @@ -942,8 +942,6 @@ int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl, > > > case V4L2_CTRL_TYPE_U8: > > > case V4L2_CTRL_TYPE_U16: > > > case V4L2_CTRL_TYPE_U32: > > > - if (ctrl->is_array) > > > - return -EINVAL; > > > ret = check_range(ctrl->type, min, max, step, def); > > > if (ret) > > > return ret; > > > @@ -960,17 +958,10 @@ int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl, > > > ctrl->default_value = def; > > > } > > > cur_to_new(ctrl); > > > - if (validate_new(ctrl, ctrl->p_new)) { > > > - if (ctrl->type == V4L2_CTRL_TYPE_INTEGER64) > > > - *ctrl->p_new.p_s64 = def; > > > - else > > > - *ctrl->p_new.p_s32 = def; > > > - } > > > + if (validate_new(ctrl, ctrl->p_new)) > > > + ctrl->type_ops->init(ctrl, 0, ctrl->p_new); > > > > Hmm, this sets *all* elements of the array to the default_value, not > > just the elements whose value is out of range. > > > > Is that what you want? Should this perhaps depend on the type of > > control? I.e. in some cases it might make sense to do this, in other > > cases it makes more sense to only adjust the elements that are out > > of range. > > > > How does that work for this TINT control? > Actually for types like INTEGER/U8/U16/U32/BOOLEAN/BUTTON/BITMASK, the function > validate_new will return zero always as well as they fix the range on the fly. > So that is ok for mlx7502x sensors. > > For types like compound/string/menu indeed, it will sets all elements of array > to default array. To fix it I propose to fix it by using the functions > std_validate_elem to check per element and then set the default manually. > But then it means, that __v4l2_ctrl_modify_range will work only for standart > v4l2 types, and not if driver use their own implementation. > > Is it fine for you? What do you think? I've another idea. If validate_new is fixing on the fly and we can't modify range for copmound/string types, so we can forbidden array for MENU types. Then validate_new will do the job for the rest types. As for now there are no needs in arrays for MENU types. What do you think?