On Tue, 19 Jul 2022 at 16:21, Benjamin Mugnier <benjamin.mugnier@xxxxxxxxxxx> wrote: > > Hi Volodymyr, > > > On 15/07/2022 17:03, Volodymyr Kharuk wrote: > > On Fri, Jul 15, 2022 at 12:36:18PM +0300, Laurent Pinchart wrote: > >> Hello, > >> > >> CC'ing Benjamin Mugnier who I recall expressed an interest in ToF > >> sensors (if I recall incorrectly, my apologies). > > I am indeed very interested. Thank you :) > > >> > >> On Fri, Jul 15, 2022 at 11:57:20AM +0300, Volodymyr Kharuk wrote: > >>> On Thu, Jul 14, 2022 at 01:31:35PM +0300, Laurent Pinchart wrote: > >>>> On Thu, Jul 14, 2022 at 11:34:46AM +0300, Volodymyr Kharuk wrote: > >>>>> Define user controls for mlx7502x driver and update MAINTAINERS > >>>>> > >>>>> Signed-off-by: Volodymyr Kharuk <vkh@xxxxxxxxxxx> > >>>>> --- > >>>>> MAINTAINERS | 7 +++++++ > >>>>> include/uapi/linux/mlx7502x.h | 31 +++++++++++++++++++++++++++++++ > >>>>> 2 files changed, 38 insertions(+) > >>>>> create mode 100644 include/uapi/linux/mlx7502x.h > >>>>> > >>>>> diff --git a/MAINTAINERS b/MAINTAINERS > >>>>> index ef3ec334fae9..1a68d888ee14 100644 > >>>>> --- a/MAINTAINERS > >>>>> +++ b/MAINTAINERS > >>>>> @@ -12673,6 +12673,13 @@ S: Supported > >>>>> W: http://www.melexis.com > >>>>> F: drivers/iio/temperature/mlx90632.c > >>>>> > >>>>> +MELEXIS MLX7502X DRIVER > >>>>> +M: Volodymyr Kharuk <vkh@xxxxxxxxxxx> > >>>>> +L: linux-media@xxxxxxxxxxxxxxx > >>>>> +S: Supported > >>>>> +W: http://www.melexis.com > >>>>> +F: include/uapi/linux/mlx7502x.h > >>>>> + > >>>>> MELFAS MIP4 TOUCHSCREEN DRIVER > >>>>> M: Sangwon Jee <jeesw@xxxxxxxxxx> > >>>>> S: Supported > >>>>> diff --git a/include/uapi/linux/mlx7502x.h b/include/uapi/linux/mlx7502x.h > >>>>> new file mode 100644 > >>>>> index 000000000000..44386f3d6f5a > >>>>> --- /dev/null > >>>>> +++ b/include/uapi/linux/mlx7502x.h > >>>>> @@ -0,0 +1,31 @@ > >>>>> +/* 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> > >>>>> + > >>>> > >>>> These controls should be documented, in > >>>> Documentation/userspace-api/media/drivers/. > >>> > >>> Ok, will do in v3 > >>> > >>>>> +/* number of phases per frame: 1..8 */ > >>>>> +#define V4L2_CID_MLX7502X_PHASE_NUMBER (V4L2_CID_USER_MLX7502X_BASE + 0) > >>>>> +/* shift of each phase in frame, this is an array of 8 elements, each 16bits */ > >>>>> +#define V4L2_CID_MLX7502X_PHASE_SEQ (V4L2_CID_USER_MLX7502X_BASE + 1) > >>>>> +/* frequency modulation in MHz */ > >>>>> +#define V4L2_CID_MLX7502X_FMOD (V4L2_CID_USER_MLX7502X_BASE + 2) > >>>>> +/* time integration of each phase in us */ > >>>>> +#define V4L2_CID_MLX7502X_TINT (V4L2_CID_USER_MLX7502X_BASE + 3) > >>>> > >>>> Are these control very device-specific, or are they concept that apply > >>>> in general to ToF sensors ? Same for V4L2_CID_MLX7502X_OUTPUT_MODE. > >>> > >>> These controls(except V4L2_CID_MLX7502X_OUTPUT_MODE) are general for ToF > >>> sensors. Do you think we should standardize them? > >> > >> I would really really like to see control standardization for ToF > >> sensors, yes :-) > > Sounds great :) > > Thanks a lot for your efforts in standardizing these controls. This is pretty close to what I expected :) > > Sensors may require multiple fmod from the user, and may not be able to deduce them from a single one. > Subframes may be acquired for each fmod (composed themselves of acquisitions for each phase), and then generate a frame from these. > Here is a quick drawing example with 2 fmod and 2 phases. Hope this makes sense. > > |-------------------------------------------------------------------------> time > |FMOD1 PHASE1|FMOD1 PHASE2|FMOD2 PHASE1|FMOD2 PHASE2|FMOD1 PHASE1|... > | SUBFRAME1 | SUBFRAME2 | > | FRAME1 | > > This allows greater ranges. > I suggest changing V4L2_CID_MLX7502X_FMOD to an array, if it suits you. > I'm curious how are you doing this? Are you using only one fmod or do you compute some others from the first one? Either in the driver or the sensor. > > >> > >> Do you know of any public litterature that explains the operating > >> principles of ToF sensors ? I don't expect most of the V4L2 developers > >> to be familiar with the concept, so something that could bring us up to > >> speed on ToF would be useful for the discussion. > > > > Here what I have: > > 1. ToF Basics from Melexis > > https://media.melexis.com/-/media/files/documents/application-notes/time-of-flight-basics-application-note-melexis.pdf > > 2. ToF Basics from TI > > https://www.ti.com/lit/wp/sloa190b/sloa190b.pdf?ts=1657842732275&ref_url=https%253A%252F%252Fwww.google.com%252F > > 2. ToF systems from TI > > https://www.ti.com/lit/ug/sbau219d/sbau219d.pdf > > 4. This more related to ToF algorithms > > https://hal.inria.fr/hal-00725654/document > > > > I hope it helps. > >> > >>> Note that the control V4L2_CID_MLX7502X_TINT is similar to > >>> V4L2_CID_EXPOSURE, but the way it is done in ToF is different. They don't > >>> have a shutter. So I gave a separate control name. Is it ok? > >> > >> Yes, I think that's fine. > >> > > Having only one integration time control is problematic for HDR sensors as they require both a short and long integration time setting. > I have the same issue for the vgxy61 camera with V4L2_CID_EXPOSURE and ended up defining 2 custom controls for both short and long exposure, but I understand this is not ideal. Maybe Laurent have an idea on this? > > >>>>> +/* mode could sw(sending i2c packet), hw(pin triggering), and continuous(self triggering) */ > >>>>> +#define V4L2_CID_MLX7502X_TRIGGER_MODE (V4L2_CID_USER_MLX7502X_BASE + 4) > >>>>> +/* in case sw or hw trigger mode is used */ > >>>>> +#define V4L2_CID_MLX7502X_TRIGGER (V4L2_CID_USER_MLX7502X_BASE + 5) > >>>> > >>>> Trigger control is likely something we need to standardize at the V4L2 > >>>> level. > >>> > >>> Ok, then I'll remove these controls for now and I will back with this as > >>> a separate patch. > >>> > >>>>> +/* 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 + 6) > >>>>> +/* ToF camers has its own temperature sensor, which can be read out only during streaming */ > >>>>> +#define V4L2_CID_MLX7502X_TEMPERATURE (V4L2_CID_USER_MLX7502X_BASE + 7) > >>>> > >>>> This should probably use the proposed temperature control from > >>>> https://lore.kernel.org/linux-media/20220415111845.27130-3-benjamin.mugnier@xxxxxxxxxxx/ > >>> > >>> Ok, then I'll remove these controls for now. > >>> > > We discussed the standardization of the temperature control with linux-hwmon subsystem team [1]. > As this happened to be a trickier problem than I thought, I decided to remove the temperature control I initially proposed. You can find the v3 of the vgxy61 without the temperature control [2]. > > So no temperature control for now. I plan on giving it another go after the vgxy61 is accepted, but for now the simpler the better. > Of course feel free to do it, I'll gratefully rebase on your work ;) I've thrown it into the ring as one of a number of image sensor related subjects to discuss at the Media Summit alongside ELCE in September[1]. Whether it makes the agenda or we get a decision there or not are different questions. An option that may be available is reading it from embedded metadata (if we had a standardised way of exporting that, which is dependent on the multiplexed streams patchset). Certainly several of the Sony image sensors that we're using drop the temperature registers in the embedded data, so we can lift it out in libcamera without needing kernel support beyond enabling it. Dave [1] https://lore.kernel.org/linux-media/CAPY8ntCwoLys1uwpoy3AW=wbBZod5cxj==z0XjUrBxK=0cwr8g@xxxxxxxxxxxxxx/ > [1] https://lore.kernel.org/linux-media/d4c868d5ef05f338bdc2237d9b9304077d268c8b.camel@xxxxxxxxxxxx/ > [2] https://lore.kernel.org/all/20220512074037.3829926-1-benjamin.mugnier@xxxxxxxxxxx/ > > >>>>> + > >>>>> +#endif /* __UAPI_MLX7502X_H_ */ > >> > >> -- > >> Regards, > >> > >> Laurent Pinchart > >
