[v2,4/6] media: uapi: Add mlx7502x header file

Commit Message

Volodymyr Kharuk July 14, 2022, 8:34 a.m. UTC

Define user controls for mlx7502x driver and update MAINTAINERS

Signed-off-by: Volodymyr Kharuk <[e-mail]>
---
 MAINTAINERS                   |  7 +++++++
 include/uapi/linux/mlx7502x.h | 31 +++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)
 create mode 100644 include/uapi/linux/mlx7502x.h

Comments

Laurent Pinchart July 14, 2022, 10:31 a.m. UTC | #1

Hi Volodymyr,

Thank you for the patch.

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 <[e-mail]>
> ---
>  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 <[e-mail]>
> +L:	[e-mail]
> +S:	Supported
> +W:	http://www.melexis.com
> +F:	include/uapi/linux/mlx7502x.h
> +
>  MELFAS MIP4 TOUCHSCREEN DRIVER
>  M:	Sangwon Jee <[e-mail]>
>  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/.

> +/* 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.

> +/* 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.

> +/* 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/[e-mail]/

> +
> +#endif /* __UAPI_MLX7502X_H_ */

Volodymyr Kharuk July 15, 2022, 8:57 a.m. UTC | #2

Hi Laurent,

Thank you for your review.

On Thu, Jul 14, 2022 at 01:31:35PM +0300, Laurent Pinchart wrote:
> Hi Volodymyr,
> 
> Thank you for the patch.
> 
> 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 <[e-mail]>
> > ---
> >  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 <[e-mail]>
> > +L:	[e-mail]
> > +S:	Supported
> > +W:	http://www.melexis.com
> > +F:	include/uapi/linux/mlx7502x.h
> > +
> >  MELFAS MIP4 TOUCHSCREEN DRIVER
> >  M:	Sangwon Jee <[e-mail]>
> >  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?

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?
> 
> > +/* 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/[e-mail]/
Ok, then I'll remove these controls for now.
> 
> > +
> > +#endif /* __UAPI_MLX7502X_H_ */
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Laurent Pinchart July 15, 2022, 9:36 a.m. UTC | #3

Hello,

CC'ing Benjamin Mugnier who I recall expressed an interest in ToF
sensors (if I recall incorrectly, my apologies).

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 <[e-mail]>
> > > ---
> > >  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 <[e-mail]>
> > > +L:	[e-mail]
> > > +S:	Supported
> > > +W:	http://www.melexis.com
> > > +F:	include/uapi/linux/mlx7502x.h
> > > +
> > >  MELFAS MIP4 TOUCHSCREEN DRIVER
> > >  M:	Sangwon Jee <[e-mail]>
> > >  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 :-)

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.

> 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.

> > > +/* 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/[e-mail]/
> 
> Ok, then I'll remove these controls for now.
> 
> > > +
> > > +#endif /* __UAPI_MLX7502X_H_ */

Volodymyr Kharuk July 15, 2022, 3:03 p.m. UTC | #4

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).
> 
> 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 <[e-mail]>
> > > > ---
> > > >  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 <[e-mail]>
> > > > +L:	[e-mail]
> > > > +S:	Supported
> > > > +W:	http://www.melexis.com
> > > > +F:	include/uapi/linux/mlx7502x.h
> > > > +
> > > >  MELFAS MIP4 TOUCHSCREEN DRIVER
> > > >  M:	Sangwon Jee <[e-mail]>
> > > >  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 :)
> 
> 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.
> 
> > > > +/* 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/[e-mail]/
> > 
> > Ok, then I'll remove these controls for now.
> > 
> > > > +
> > > > +#endif /* __UAPI_MLX7502X_H_ */
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Benjamin Mugnier July 19, 2022, 3:20 p.m. UTC | #5

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 <[e-mail]>
>>>>> ---
>>>>>  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 <[e-mail]>
>>>>> +L:	[e-mail]
>>>>> +S:	Supported
>>>>> +W:	http://www.melexis.com
>>>>> +F:	include/uapi/linux/mlx7502x.h
>>>>> +
>>>>>  MELFAS MIP4 TOUCHSCREEN DRIVER
>>>>>  M:	Sangwon Jee <[e-mail]>
>>>>>  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/[e-mail]/
>>>
>>> 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 ;)

[1] https://lore.kernel.org/linux-media/[e-mail]/
[2] https://lore.kernel.org/all/[e-mail]/

>>>>> +
>>>>> +#endif /* __UAPI_MLX7502X_H_ */
>>
>> -- 
>> Regards,
>>
>> Laurent Pinchart
>

Dave Stevenson July 19, 2022, 3:31 p.m. UTC | #6

On Tue, 19 Jul 2022 at 16:21, Benjamin Mugnier
<[e-mail]> 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 <[e-mail]>
> >>>>> ---
> >>>>>  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 <[e-mail]>
> >>>>> +L:       [e-mail]
> >>>>> +S:       Supported
> >>>>> +W:       http://www.melexis.com
> >>>>> +F:       include/uapi/linux/mlx7502x.h
> >>>>> +
> >>>>>  MELFAS MIP4 TOUCHSCREEN DRIVER
> >>>>>  M:       Sangwon Jee <[e-mail]>
> >>>>>  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/[e-mail]/
> >>>
> >>> 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=[e-mail]/

> [1] https://lore.kernel.org/linux-media/[e-mail]/
> [2] https://lore.kernel.org/all/[e-mail]/
>
> >>>>> +
> >>>>> +#endif /* __UAPI_MLX7502X_H_ */
> >>
> >> --
> >> Regards,
> >>
> >> Laurent Pinchart
> >

Volodymyr Kharuk July 20, 2022, 2:44 p.m. UTC | #7

Hi Benjamin,

On Tue, Jul 19, 2022 at 05:20:40PM +0200, Benjamin Mugnier 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 <[e-mail]>
> >>>>> ---
> >>>>>  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 <[e-mail]>
> >>>>> +L:	[e-mail]
> >>>>> +S:	Supported
> >>>>> +W:	http://www.melexis.com
> >>>>> +F:	include/uapi/linux/mlx7502x.h
> >>>>> +
> >>>>>  MELFAS MIP4 TOUCHSCREEN DRIVER
> >>>>>  M:	Sangwon Jee <[e-mail]>
> >>>>>  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                      |
Just to be clear: one camera trigger generates one frame.
The number of subframes can be configured via register.
One phase generates one image. Do we have the same understanding?
> 
> 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.
The mlx7502x supports only one fmod per frame. Instead it is possible to use
a few phase shifts(V4L2_CID_MLX7502X_PHASE_SEQ) per one frame. Using
cross-correlation it is possible to calculate the distance from received images.
> 
> >>
> >> 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?
It is possible to have integration time per phase. For now I just copy tint into all phase registers.
> 
> >>>>> +/* 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/[e-mail]/
> >>>
> >>> 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 ;)
> 
> [1] https://lore.kernel.org/linux-media/[e-mail]/
> [2] https://lore.kernel.org/all/[e-mail]/
> 
> >>>>> +
> >>>>> +#endif /* __UAPI_MLX7502X_H_ */
> >>
> >> -- 
> >> Regards,
> >>
> >> Laurent Pinchart
> >

Benjamin Mugnier July 21, 2022, 9:58 a.m. UTC | #8

Hi Volodymyr,

On 20/07/2022 16:44, Volodymyr Kharuk wrote:
> Hi Benjamin,
> 
> On Tue, Jul 19, 2022 at 05:20:40PM +0200, Benjamin Mugnier 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 <[e-mail]>
>>>>>>> ---
>>>>>>>  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 <[e-mail]>
>>>>>>> +L:	[e-mail]
>>>>>>> +S:	Supported
>>>>>>> +W:	http://www.melexis.com
>>>>>>> +F:	include/uapi/linux/mlx7502x.h
>>>>>>> +
>>>>>>>  MELFAS MIP4 TOUCHSCREEN DRIVER
>>>>>>>  M:	Sangwon Jee <[e-mail]>
>>>>>>>  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                      |
> Just to be clear: one camera trigger generates one frame.
> The number of subframes can be configured via register.

Yes exactly. It's mostly vocabulary differences afaik, but we are on the same 

> One phase generates one image. Do we have the same understanding?

Yes, one frame on the MIPI is one phase. The reconstitution is done later on.
But some sensors tend to put their "start of frame" and "end of frame" markers for the concatenation of subframes (the frame on my drawing), and not for each phase. I'm not really familiar with this yet.
This is used example if you want to plug your sensor in a software stack that can't handle more than a limited number of fps. You trick it into getting one big frame and then slice after. This is afaik the same mechanism as used for high fps cameras. I don't really know how to handle this kind of usage yet.

>>
>> 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.
> The mlx7502x supports only one fmod per frame. Instead it is possible to use
> a few phase shifts(V4L2_CID_MLX7502X_PHASE_SEQ) per one frame. Using
> cross-correlation it is possible to calculate the distance from received images.

Alright. In my opinion if we standardize this control we could make it an array from the start to prepare for other sensors using multiple fmod.

>>
>>>>
>>>> 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?
> It is possible to have integration time per phase. For now I just copy tint into all phase registers.

Yes I see little to no use cases for variable integration time per phase. So your control is fine.

For HDR it's kind of special, as every frame is acquired is in fact 2 frames: one in HDR and one in SDR, and they require 2 different integration time.
As I said the problem already exists for camera sensors, so this is fine as is for now :)

>>
>>>>>>> +/* 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/[e-mail]/
>>>>>
>>>>> 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 ;)
>>
>> [1] https://lore.kernel.org/linux-media/[e-mail]/
>> [2] https://lore.kernel.org/all/[e-mail]/
>>
>>>>>>> +
>>>>>>> +#endif /* __UAPI_MLX7502X_H_ */
>>>>
>>>> -- 
>>>> Regards,
>>>>
>>>> Laurent Pinchart
>>>
>

Patch

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 <[e-mail]>
+L:	[e-mail]
+S:	Supported
+W:	http://www.melexis.com
+F:	include/uapi/linux/mlx7502x.h
+
 MELFAS MIP4 TOUCHSCREEN DRIVER
 M:	Sangwon Jee <[e-mail]>
 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>
+
+/* 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)
+/* 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)
+/* 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)
+
+#endif /* __UAPI_MLX7502X_H_ */