media: i2c: mlx7502x ToF camera support
v2 cover media: i2c: mlx7502x ToF camera support
Hello,
This series adds support for the Melexis 75026 and 75027 Time of Flight camera sensors, with DT bindings in patch 5/6 and a driver in patch 6/6. Also it adds 1X12 greyscale format to xilinx MIPI receiver in patch 1/6 and to xilinx video in patch 2/6.
v2:
- added external clock to the sensor
- added all regulators required by the sensor
- added posibility to choose sensor type in device tree
- added prefixes to all custom types in device tree and driver as well
- style fixes
Volodymyr Kharuk (6):
media: xilinx: csi2rxss: Add 1X12 greyscale format
media: xilinx: video: Add 1X12 greyscale format
media: v4l: ctrls: Add user control base for mlx7502x
media: uapi: Add mlx7502x header file
media: dt-bindings: media: i2c: Add mlx7502x camera sensor binding
media: i2c: Add driver for mlx7502x ToF sensor
.../bindings/media/i2c/melexis,mlx7502x.yaml | 146 ++ MAINTAINERS | 9 + drivers/media/i2c/Kconfig | 13 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/mlx7502x.c | 2020 +++++++++++++++++ .../media/platform/xilinx/xilinx-csi2rxss.c | 1 + drivers/media/platform/xilinx/xilinx-vip.c | 2 + include/uapi/linux/mlx7502x.h | 31 + include/uapi/linux/v4l2-controls.h | 6 + 9 files changed, 2229 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml create mode 100644 drivers/media/i2c/mlx7502x.c create mode 100644 include/uapi/linux/mlx7502x.h
base-commit: d8e8aa866ed8636fd6c1017c3d9453eab2922496
Signature · 2 lines
BR, Volodymyr Kharuk
PATCH v2 1/6 media: xilinx: csi2rxss: Add 1X12 greyscale format
Extend the csi2rxss with Y12_1X12 greyscale format
--- drivers/media/platform/xilinx/xilinx-csi2rxss.c | 1 + 1 file changed, 1 insertion(+)
drivers/media/platform/xilinx/xilinx-csi2rxss.c +1 -0
@@ -188,6 +188,7 @@ static const u32 xcsi2dt_mbus_lut[][2] = { { MIPI_CSI2_DT_RAW12, MEDIA_BUS_FMT_SBGGR12_1X12 }, { MIPI_CSI2_DT_RAW12, MEDIA_BUS_FMT_SGBRG12_1X12 }, { MIPI_CSI2_DT_RAW12, MEDIA_BUS_FMT_SGRBG12_1X12 }, + { MIPI_CSI2_DT_RAW12, MEDIA_BUS_FMT_Y12_1X12 }, { MIPI_CSI2_DT_RAW16, MEDIA_BUS_FMT_SRGGB16_1X16 }, { MIPI_CSI2_DT_RAW16, MEDIA_BUS_FMT_SBGGR16_1X16 }, { MIPI_CSI2_DT_RAW16, MEDIA_BUS_FMT_SGBRG16_1X16 },
Signature · 2 lines
BR, Volodymyr Kharuk
PATCH v2 2/6 media: xilinx: video: Add 1X12 greyscale format
In reply to Volodymyr KharukExtend the xilinx video driver with Y12_1X12 greyscale format
--- drivers/media/platform/xilinx/xilinx-vip.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/media/platform/xilinx/xilinx-vip.c +2 -0
@@ -40,6 +40,8 @@ static const struct xvip_video_format xvip_video_formats[] = { 1, V4L2_PIX_FMT_SGBRG8 }, {XVIP_VF_MONO_SENSOR ,8 ,"bggr" ,MEDIA_BUS_FMT_SBGGR8_1X8 , 1, V4L2_PIX_FMT_SBGGR8 }, + { XVIP_VF_MONO_SENSOR, 12, "mono", MEDIA_BUS_FMT_Y12_1X12, + 2, V4L2_PIX_FMT_Y12 }, };/**
Signature · 2 lines
BR, Volodymyr Kharuk
PATCH v2 3/6 media: v4l: ctrls: Add user control base for mlx7502x
In reply to Volodymyr KharukAdd a control base for mlx7502x and reserve 16 controls. ToF sensor requires a bunch of new user controls.
--- include/uapi/linux/v4l2-controls.h | 6 ++++++ 1 file changed, 6 insertions(+)
include/uapi/linux/v4l2-controls.h +6 -0
@@ -225,6 +225,12 @@ enum v4l2_colorfx {*/ #define V4L2_CID_USER_ISL7998X_BASE (V4L2_CID_USER_BASE + 0x1180) +/* +* The base for Melexis ToF 7502x driver controls. +* We reserve 16 controls for this driver. +*/ +#define V4L2_CID_USER_MLX7502X_BASE (V4L2_CID_USER_BASE + 0x1190) +/* MPEG-class control IDs */ /* The MPEG controls are applicable to all codec controls * and the 'MPEG' part of the define is historical */
Signature · 2 lines
BR, Volodymyr Kharuk
PATCH v2 4/6 media: uapi: Add mlx7502x header file
In reply to Volodymyr KharukDefine user controls for mlx7502x driver and update MAINTAINERS
--- MAINTAINERS | 7 +++++++ include/uapi/linux/mlx7502x.h | 31 +++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100644 include/uapi/linux/mlx7502x.h
MAINTAINERS +7 -0
@@ -12673,6 +12673,13 @@ S: Supported W: http://www.melexis.com F: drivers/iio/temperature/mlx90632.c +MELEXIS MLX7502X DRIVER +M: Volodymyr Kharuk <[email protected]> +L: [email protected] +S: Supported +W: http://www.melexis.com +F: include/uapi/linux/mlx7502x.h + MELFAS MIP4 TOUCHSCREEN DRIVER M: Sangwon Jee <[email protected]> S: Supported
include/uapi/linux/mlx7502x.h new file +31 -0
@@ -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_ */
Signature · 2 lines
BR, Volodymyr Kharuk
PATCH v2 5/6 media: dt-bindings: media: i2c: Add mlx7502x camera sensor binding
In reply to Volodymyr KharukAdd device tree binding of the mlx7502x and update MAINTAINERS
--- .../bindings/media/i2c/melexis,mlx7502x.yaml | 146 ++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 147 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +146 -0
@@ -0,0 +1,146 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/melexis,mlx7502x.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Melexis ToF 7502x MIPI CSI-2 Sensor + +maintainers: + - Volodymyr Kharuk <[email protected]> + +description: |- + Melexis ToF 7502x sensors has a CSI-2 output. It supports 2 and 4 lanes, + and mipi speeds are 300, 600, 704, 800, 904, 960Mbs. Supported format is RAW12. + Sensor 75026 is QVGA, while 75027 is VGA sensor. + If you use compatible = "melexis,mlx7502x", then autodetect will be called. + +properties: + compatible: + items: + - enum: + - melexis,mlx7502x + - melexis,mlx75026 + - melexis,mlx75027 + + reg: + maxItems: 1 + + assigned-clocks: true + assigned-clock-parents: true + assigned-clock-rates: true + + clocks: + description: Clock frequency 8MHz + maxItems: 1 + + vdda-supply: + description: + Definition of the regulator used as analog power supply(2.7V). + + vddif-supply: + description: + Definition of the regulator used as interface power supply(1.8V). + + vddd-supply: + description: + Definition of the regulator used as digital power supply(1.2V). + + vdmix-supply: + description: + Definition of the regulator used as mixed driver power supply(1.2V). + + reset-gpios: + maxItems: 1 + description: Reset Sensor GPIO Control (active low) + + melexis,trig-gpios: + maxItems: 1 + description: + Hardware Trigger GPIO Control (active low) + When the hardware trigger mode is enabled, this GPIO is used to generate + a low level impulse for about 100us. The impulse triggers a new + measurement cycle. + + melexis,leden-gpios: + maxItems: 1 + description: + Led driver enable GPIO Control (active high) + This GPIO notifies the illumination driver when it is safe to use led + signals from the sensor. + + port: + description: MIPI CSI-2 transmitter port + $ref: /schemas/graph.yaml#/$defs/port-base + + properties: + endpoint: + $ref: /schemas/media/video-interfaces.yaml# + unevaluatedProperties: false + + properties: + data-lanes: + oneOf: + - items: + - const: 1 + - const: 2 + - items: + - const: 1 + - const: 2 + - const: 3 + - const: 4 + + clock-noncontinuous: true + link-frequencies: true + + required: + - data-lanes + - link-frequencies + +required: + - compatible + - reg + - clocks + - port + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + + i2c { + #address-cells = <1>; + #size-cells = <0>; + + mlx7502x: camera@57 { + compatible = "melexis,mlx7502x"; + reg = <0x57>; + clocks = <&mlx7502x_clk>; + + assigned-clocks = <&mlx7502x_clk>; + assigned-clock-parents = <&mlx7502x_clk_parent>; + assigned-clock-rates = <8000000>; + + vddd-supply = <&mlx_7502x_reg>; + + reset-gpios = <&gpio_exp 6 GPIO_ACTIVE_HIGH>; + melexis,trig-gpios = <&gpio_exp 2 GPIO_ACTIVE_HIGH>; + melexis,leden-gpios = <&gpio_exp 7 GPIO_ACTIVE_HIGH>; + + port { + mlx7502x_out_mipi_csi2: endpoint { + remote-endpoint = <&mipi_csi2_from_mlx7502x>; + data-lanes = <1 2 3 4>; + link-frequencies = /bits/ 64 < 960000000 + 904000000 + 800000000 + 704000000 + 600000000 + 300000000 >; + }; + }; + }; + }; + +...
MAINTAINERS +1 -0
@@ -12678,6 +12678,7 @@ M: Volodymyr Kharuk <[email protected]> L: [email protected] S: Supported W: http://www.melexis.com +F: Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml F: include/uapi/linux/mlx7502x.h MELFAS MIP4 TOUCHSCREEN DRIVER
Signature · 2 lines
BR, Volodymyr Kharuk
PATCH v2 6/6 media: i2c: Add driver for mlx7502x ToF sensor
In reply to Volodymyr KharukMelexis ToF 7502x sensors has a CSI-2 output. It supports 2 and 4 lanes, and mipi speeds are 300, 600, 704, 800, 904, 960Mbs. Supported format is RAW12.
Sensor 75026 is QVGA, while 75027 is VGA sensor.
The driver supports:
- Digital binning and cropping
- Horizontal and vertical flip
- phase shifts(up to 8)
- frequency modulation
- time integration
- internal temperature
- different trigger sources
--- MAINTAINERS | 1 + drivers/media/i2c/Kconfig | 13 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/mlx7502x.c | 2020 ++++++++++++++++++++++++++++++++++ 4 files changed, 2035 insertions(+) create mode 100644 drivers/media/i2c/mlx7502x.c
MAINTAINERS +1 -0
@@ -12679,6 +12679,7 @@ L: [email protected] S: Supported W: http://www.melexis.com F: Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml +F: drivers/media/i2c/mlx7502x.c F: include/uapi/linux/mlx7502x.h MELFAS MIP4 TOUCHSCREEN DRIVER
drivers/media/i2c/Kconfig +13 -0
@@ -216,6 +216,19 @@ config VIDEO_IMX412 config VIDEO_MAX9271_LIB tristate +config VIDEO_MLX7502X + tristate "Melexis ToF 75026 and 75027 sensors support" + depends on I2C && VIDEO_DEV + select MEDIA_CONTROLLER + select VIDEO_V4L2_SUBDEV_API + select V4L2_FWNODE + help + This is a V4L2 sensor driver for the Melexis 75026 and 75027 + ToF sensors. + + To compile this driver as a module, choose M here: the + module will be called mlx7502x. + config VIDEO_MT9M001 tristate "mt9m001 support" depends on I2C && VIDEO_DEV
drivers/media/i2c/Makefile +1 -0
@@ -57,6 +57,7 @@ obj-$(CONFIG_VIDEO_M5MOLS) += m5mols/ obj-$(CONFIG_VIDEO_MAX9271_LIB) += max9271.o obj-$(CONFIG_VIDEO_MAX9286) += max9286.o obj-$(CONFIG_VIDEO_ML86V7667) += ml86v7667.o +obj-$(CONFIG_VIDEO_MLX7502X) += mlx7502x.o obj-$(CONFIG_VIDEO_MSP3400) += msp3400.o obj-$(CONFIG_VIDEO_MT9M001) += mt9m001.o obj-$(CONFIG_VIDEO_MT9M032) += mt9m032.o
drivers/media/i2c/mlx7502x.c new file +2020 -0
2,020 lines not on this page · raw message
Signature · 2 lines
BR, Volodymyr Kharuk
Re: [PATCH v2 5/6] media: dt-bindings: media: i2c: Add mlx7502x camera sensor binding
In reply to Volodymyr KharukQuoted 3 lines · Volodymyr Kharuk
Add device tree binding of the mlx7502x and update
MAINTAINERSSigned-off-by Volodymyr Kharuk <[email protected]>
Wrong subject prefix. Remove trailing "binding".
Quoted 22 lines
--- .../bindings/media/i2c/melexis,mlx7502x.yaml | 146 ++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 147 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yamlDocumentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +10
-0@@ -0,0 +1,146 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/melexis,mlx7502x.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Melexis ToF 7502x MIPI CSI-2 Sensor + +maintainers: + - Volodymyr Kharuk <[email protected]>
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml line 20
Quoted 10 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +10
-0+ +description: |- + Melexis ToF 7502x sensors has a CSI-2 output. It supports 2 and 4 lanes, + and mipi speeds are 300, 600, 704, 800, 904, 960Mbs. Supported format is RAW12. + Sensor 75026 is QVGA, while 75027 is VGA sensor. + If you use compatible = "melexis,mlx7502x", then autodetect will be called. + +properties: + compatible: + items:
No items. You have just one item.
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml line 31
Quoted 11 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +11
-0+ - enum: + - melexis,mlx7502x + - melexis,mlx75026 + - melexis,mlx75027 + + reg: + maxItems: 1 + + assigned-clocks: true + assigned-clock-parents: true + assigned-clock-rates: true
These are not needed.
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml line 39
Quoted 8 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +8
-0+ + clocks: + description: Clock frequency 8MHz + maxItems: 1 + + vdda-supply: + description: + Definition of the regulator used as analog power supply(2.7V).
s/Definition of the regulator used as //
(redundant)
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml line 43
Quoted 4 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +4
-0+ + vddif-supply: + description: + Definition of the regulator used as interface power supply(1.8V).
s/Definition of the regulator used as //
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml line 47
Quoted 4 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +4
-0+ + vddd-supply: + description: + Definition of the regulator used as digital power supply(1.2V).
s/Definition of the regulator used as //
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml line 51
Quoted 4 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +4
-0+ + vdmix-supply: + description: + Definition of the regulator used as mixed driver power supply(1.2V).
s/Definition of the regulator used as //
Quoted 33 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +33
-0+ + reset-gpios: + maxItems: 1 + description: Reset Sensor GPIO Control (active low) + + melexis,trig-gpios: + maxItems: 1 + description: + Hardware Trigger GPIO Control (active low) + When the hardware trigger mode is enabled, this GPIO is used to generate + a low level impulse for about 100us. The impulse triggers a new + measurement cycle. + + melexis,leden-gpios: + maxItems: 1 + description: + Led driver enable GPIO Control (active high) + This GPIO notifies the illumination driver when it is safe to use led + signals from the sensor. + + port: + description: MIPI CSI-2 transmitter port + $ref: /schemas/graph.yaml#/$defs/port-base + + properties: + endpoint: + $ref: /schemas/media/video-interfaces.yaml# + unevaluatedProperties: false + + properties: + data-lanes: + oneOf: + - items:
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml line 94
Quoted 10 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +10
-0+ - const: 1 + - const: 2 + - items: + - const: 1 + - const: 2 + - const: 3 + - const: 4 + + clock-noncontinuous: true + link-frequencies: true
You do not need these two, they come from video-interfaces.yaml
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml line 105
Quoted 11 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +11
-0+ + required: + - data-lanes + - link-frequencies + +required: + - compatible + - reg + - clocks + - port +
Best regards,
Krzysztof
Laurent Pinchart
maintainerRe: [PATCH v2 5/6] media: dt-bindings: media: i2c: Add mlx7502x camera sensor binding
In reply to Volodymyr KharukHi Volodymyr,
Thank you for the patch.
Quoted 21 lines · Volodymyr Kharuk
Add device tree binding of the mlx7502x and update
MAINTAINERSSigned-off-by Volodymyr Kharuk <[email protected]>--- .../bindings/media/i2c/melexis,mlx7502x.yaml | 146 ++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 147 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yamlDocumentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +6
-0@@ -0,0 +1,146 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/melexis,mlx7502x.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# +
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml line 16
Quoted 10 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +10
-0+title: Melexis ToF 7502x MIPI CSI-2 Sensor + +maintainers: + - Volodymyr Kharuk <[email protected]> + +description: |- + Melexis ToF 7502x sensors has a CSI-2 output. It supports 2 and 4 lanes, + and mipi speeds are 300, 600, 704, 800, 904, 960Mbs. Supported format is RAW12. + Sensor 75026 is QVGA, while 75027 is VGA sensor. + If you use compatible = "melexis,mlx7502x", then autodetect will be called.
I'd move this last line as a description of the compatible property, but I'm also not sure this should be mentioned in the DT bindings, as it's a driver implementation detail. I'm actually not sure we should support it with three different compatible values as proposed, as without this documentation users will have a hard time figuring out what compatible value to pick.
One option would be to support the following three compatible values:
compatible = "melexis,mlx75026", "melexis,mlx7502x"; compatible = "melexis,mlx75027", "melexis,mlx7502x"; compatible = "melexis,mlx7502x";
The last one only would trigger autodetection. I'm still not sure how to document that properly in bindings though.
Quoted 44 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +44
-0+ +properties: + compatible: + items: + - enum: + - melexis,mlx7502x + - melexis,mlx75026 + - melexis,mlx75027 + + reg: + maxItems: 1 + + assigned-clocks: true + assigned-clock-parents: true + assigned-clock-rates: true + + clocks: + description: Clock frequency 8MHz + maxItems: 1 + + vdda-supply: + description: + Definition of the regulator used as analog power supply(2.7V). + + vddif-supply: + description: + Definition of the regulator used as interface power supply(1.8V). + + vddd-supply: + description: + Definition of the regulator used as digital power supply(1.2V). + + vdmix-supply: + description: + Definition of the regulator used as mixed driver power supply(1.2V). + + reset-gpios: + maxItems: 1 + description: Reset Sensor GPIO Control (active low) + + melexis,trig-gpios: + maxItems: 1 + description: + Hardware Trigger GPIO Control (active low)
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml line 70
Quoted 10 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +10
-0+ When the hardware trigger mode is enabled, this GPIO is used to generate + a low level impulse for about 100us. The impulse triggers a new + measurement cycle. + + melexis,leden-gpios: + maxItems: 1 + description: + Led driver enable GPIO Control (active high) + This GPIO notifies the illumination driver when it is safe to use led + signals from the sensor.
As far as I understand this isn't an input of the sensor, but a signal that is driven by the driver and controls the separate illuminator. It shouldn't be specified in this binding, as it's not a property of the sensor. You should instead have a DT node for the illumination driver.
Quoted 23 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +23
-0+ port: + description: MIPI CSI-2 transmitter port + $ref: /schemas/graph.yaml#/$defs/port-base + + properties: + endpoint: + $ref: /schemas/media/video-interfaces.yaml# + unevaluatedProperties: false + + properties: + data-lanes: + oneOf: + - items: + - const: 1 + - const: 2 + - items: + - const: 1 + - const: 2 + - const: 3 + - const: 4 + + clock-noncontinuous: true + link-frequencies: true
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml line 104
Quoted 10 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +10
-0+ + required: + - data-lanes + - link-frequencies + +required: + - compatible + - reg + - clocks + - port
Let's make the supplies mandatory too, as the chip can't operate without them.
Quoted 54 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +42
-0+ +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + + i2c { + #address-cells = <1>; + #size-cells = <0>; + + mlx7502x: camera@57 { + compatible = "melexis,mlx7502x"; + reg = <0x57>; + clocks = <&mlx7502x_clk>; + + assigned-clocks = <&mlx7502x_clk>; + assigned-clock-parents = <&mlx7502x_clk_parent>; + assigned-clock-rates = <8000000>; + + vddd-supply = <&mlx_7502x_reg>; + + reset-gpios = <&gpio_exp 6 GPIO_ACTIVE_HIGH>; + melexis,trig-gpios = <&gpio_exp 2 GPIO_ACTIVE_HIGH>; + melexis,leden-gpios = <&gpio_exp 7 GPIO_ACTIVE_HIGH>; + + port { + mlx7502x_out_mipi_csi2: endpoint { + remote-endpoint = <&mipi_csi2_from_mlx7502x>; + data-lanes = <1 2 3 4>; + link-frequencies = /bits/ 64 < 960000000 + 904000000 + 800000000 + 704000000 + 600000000 + 300000000 >; + }; + }; + }; + }; + +...MAINTAINERS +1
-0@@ -12678,6 +12678,7 @@ M: Volodymyr Kharuk <[email protected]> L: [email protected] S: Supported W: http://www.melexis.com +F: Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml F: include/uapi/linux/mlx7502x.h MELFAS MIP4 TOUCHSCREEN DRIVER
Signature · 3 lines
Regards, Laurent Pinchart
Laurent Pinchart
maintainerRe: [PATCH v2 4/6] media: uapi: Add mlx7502x header file
In reply to Volodymyr KharukHi Volodymyr,
Thank you for the patch.
Quoted 36 lines · Volodymyr Kharuk
Define user controls for mlx7502x driver and update
MAINTAINERSSigned-off-by Volodymyr Kharuk <[email protected]>--- MAINTAINERS | 7 +++++++ include/uapi/linux/mlx7502x.h | 31 +++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100644 include/uapi/linux/mlx7502x.hMAINTAINERS +7
-0@@ -12673,6 +12673,13 @@ S: Supported W: http://www.melexis.com F: drivers/iio/temperature/mlx90632.c +MELEXIS MLX7502X DRIVER +M: Volodymyr Kharuk <[email protected]> +L: [email protected] +S: Supported +W: http://www.melexis.com +F: include/uapi/linux/mlx7502x.h + MELFAS MIP4 TOUCHSCREEN DRIVER M: Sangwon Jee <[email protected]> S: Supportedinclude/uapi/linux/mlx7502x.h new file +3
-0@@ -0,0 +1,31 @@ +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ +/* +* Melexis 7502x ToF cameras driver.
include/uapi/linux/mlx7502x.h line 13
Quoted 10 lines
include/uapi/linux/mlx7502x.h new file +10
-0+* +* Copyright (C) 2021 Melexis N.V. +* +*/ + +#ifndef __UAPI_MLX7502X_H_ +#define __UAPI_MLX7502X_H_ + +#include <linux/v4l2-controls.h> +
These controls should be documented, inDocumentation/userspace-api/media/drivers/.
include/uapi/linux/mlx7502x.h line 21
Quoted 8 lines
include/uapi/linux/mlx7502x.h new file +8
-0+/* 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.
include/uapi/linux/mlx7502x.h line 25
Quoted 4 lines
include/uapi/linux/mlx7502x.h new file +4
-0+/* 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.
include/uapi/linux/mlx7502x.h line 29
Quoted 4 lines
include/uapi/linux/mlx7502x.h new file +4
-0+/* 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/[email protected]/
Quoted 2 lines
include/uapi/linux/mlx7502x.h new file +2
-0+ +#endif /* __UAPI_MLX7502X_H_ */
Signature · 3 lines
Regards, Laurent Pinchart
Re: [PATCH v2 1/6] media: xilinx: csi2rxss: Add 1X12 greyscale format
In reply to Volodymyr KharukHi Volodymyr,
Thank you for the patch.
Quoted 3 lines · Volodymyr Kharuk
Extend the csi2rxss with
Y12_1X12greyscale formatSigned-off-by Volodymyr Kharuk <[email protected]>
Quoted 16 lines
--- drivers/media/platform/xilinx/xilinx-csi2rxss.c | 1 + 1 file changed, 1 insertion(+)drivers/media/platform/xilinx/xilinx-csi2rxss.c +1
-0@@ -188,6 +188,7 @@ static const u32 xcsi2dt_mbus_lut[][2] = { { MIPI_CSI2_DT_RAW12, MEDIA_BUS_FMT_SBGGR12_1X12 }, { MIPI_CSI2_DT_RAW12, MEDIA_BUS_FMT_SGBRG12_1X12 }, { MIPI_CSI2_DT_RAW12, MEDIA_BUS_FMT_SGRBG12_1X12 }, + { MIPI_CSI2_DT_RAW12, MEDIA_BUS_FMT_Y12_1X12 }, { MIPI_CSI2_DT_RAW16, MEDIA_BUS_FMT_SRGGB16_1X16 }, { MIPI_CSI2_DT_RAW16, MEDIA_BUS_FMT_SBGGR16_1X16 }, { MIPI_CSI2_DT_RAW16, MEDIA_BUS_FMT_SGBRG16_1X16 },
Signature · 3 lines
Regards, Laurent Pinchart
Re: [PATCH v2 5/6] media: dt-bindings: media: i2c: Add mlx7502x camera sensor binding
In reply to Laurent PinchartQuoted 44 lines · Laurent Pinchart
Hi Volodymyr,
Thank you for the patch.
Quoted 21 lines · Volodymyr Kharuk
Add device tree binding of the mlx7502x and update
MAINTAINERSSigned-off-by Volodymyr Kharuk <[email protected]>--- .../bindings/media/i2c/melexis,mlx7502x.yaml | 146 ++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 147 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yamlDocumentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +6
-0@@ -0,0 +1,146 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/melexis,mlx7502x.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# +Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml line 16
Quoted 10 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +10
-0+title: Melexis ToF 7502x MIPI CSI-2 Sensor + +maintainers: + - Volodymyr Kharuk <[email protected]> + +description: |- + Melexis ToF 7502x sensors has a CSI-2 output. It supports 2 and 4 lanes, + and mipi speeds are 300, 600, 704, 800, 904, 960Mbs. Supported format is RAW12. + Sensor 75026 is QVGA, while 75027 is VGA sensor. + If you use compatible = "melexis,mlx7502x", then autodetect will be called.I'd move this last line as a description of the compatible property, but I'm also not sure this should be mentioned in the DT bindings, as it's a driver implementation detail. I'm actually not sure we should support it with three different compatible values as proposed, as without this documentation users will have a hard time figuring out what compatible value to pick.
Quoted 8 lines
One option would be to support the following three compatible values:
compatible = "melexis,mlx75026", "melexis,mlx7502x"; compatible = "melexis,mlx75027", "melexis,mlx7502x"; compatible = "melexis,mlx7502x";The last one only would trigger autodetection. I'm still not sure how to document that properly in bindings though.
I missed that part of binding.
Wildcards are not allowed in compatible, so mlx7502x has to go.
Anyway what does this autodetection mean?
Best regards,
Krzysztof
Re: [PATCH v2 2/6] media: xilinx: video: Add 1X12 greyscale format
In reply to Volodymyr KharukHi Volodymyr,
Thank you for the patch.
Quoted 3 lines · Volodymyr Kharuk
Extend the xilinx video driver with
Y12_1X12greyscale formatSigned-off-by Volodymyr Kharuk <[email protected]>
Quoted 17 lines
--- drivers/media/platform/xilinx/xilinx-vip.c | 2 ++ 1 file changed, 2 insertions(+)drivers/media/platform/xilinx/xilinx-vip.c +2
-0@@ -40,6 +40,8 @@ static const struct xvip_video_format xvip_video_formats[] = { 1, V4L2_PIX_FMT_SGBRG8 }, {XVIP_VF_MONO_SENSOR ,8 ,"bggr" ,MEDIA_BUS_FMT_SBGGR8_1X8 , 1, V4L2_PIX_FMT_SBGGR8 }, + { XVIP_VF_MONO_SENSOR, 12, "mono", MEDIA_BUS_FMT_Y12_1X12, + 2, V4L2_PIX_FMT_Y12 }, };/**
Signature · 3 lines
Regards, Laurent Pinchart
Laurent Pinchart
maintainerRe: [PATCH v2 5/6] media: dt-bindings: media: i2c: Add mlx7502x camera sensor binding
In reply to Krzysztof KozlowskiQuoted 51 lines · Krzysztof Kozlowski
Quoted 50 lines · Laurent Pinchart
Hi Volodymyr,
Thank you for the patch.
Quoted 21 lines · Volodymyr Kharuk
Add device tree binding of the mlx7502x and update
MAINTAINERSSigned-off-by Volodymyr Kharuk <[email protected]>--- .../bindings/media/i2c/melexis,mlx7502x.yaml | 146 ++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 147 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yamlDocumentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +6
-0@@ -0,0 +1,146 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/melexis,mlx7502x.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# +Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml line 16
Quoted 10 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +10
-0+title: Melexis ToF 7502x MIPI CSI-2 Sensor + +maintainers: + - Volodymyr Kharuk <[email protected]> + +description: |- + Melexis ToF 7502x sensors has a CSI-2 output. It supports 2 and 4 lanes, + and mipi speeds are 300, 600, 704, 800, 904, 960Mbs. Supported format is RAW12. + Sensor 75026 is QVGA, while 75027 is VGA sensor. + If you use compatible = "melexis,mlx7502x", then autodetect will be called.I'd move this last line as a description of the compatible property, but I'm also not sure this should be mentioned in the DT bindings, as it's a driver implementation detail. I'm actually not sure we should support it with three different compatible values as proposed, as without this documentation users will have a hard time figuring out what compatible value to pick.
One option would be to support the following three compatible values:
compatible = "melexis,mlx75026", "melexis,mlx7502x"; compatible = "melexis,mlx75027", "melexis,mlx7502x"; compatible = "melexis,mlx7502x";
Quoted 6 lines
Quoted 2 lines
The last one only would trigger autodetection. I'm still not sure how to document that properly in bindings though.
I missed that part of binding.
Wildcards are not allowed in compatible, so mlx7502x has to go.
Really ? We've had fallback generic compatible strings since the beginning.
Quoted 1 line
Anyway what does this autodetection mean?
As far as I understand, it means that the driver will use a hardware identification register to figure out if the sensor is a 75026 or 75027. The upside is that one doesn't need to change the device tree when swapping between those two sensors. The downside is that the sensor needs to be powered up at probe time. Depending on the platform, one of those two behaviours is preferred. Auto-detection is nice, but in laptops or tablets (not a use case for this particular device, but the problem applies to camera sensors in general), it would mean that the privacy LED of the camera could be briefly lit at boot time due to the sensor being powered on, which can worry users.
Signature · 3 lines
Regards, Laurent Pinchart
Re: [PATCH v2 5/6] media: dt-bindings: media: i2c: Add mlx7502x camera sensor binding
Quoted 55 lines · Laurent Pinchart
Quoted 54 lines · Krzysztof Kozlowski
Quoted 52 lines · Laurent Pinchart
Hi Volodymyr,
Thank you for the patch.
Quoted 21 lines · Volodymyr Kharuk
Add device tree binding of the mlx7502x and update
MAINTAINERSSigned-off-by Volodymyr Kharuk <[email protected]>--- .../bindings/media/i2c/melexis,mlx7502x.yaml | 146 ++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 147 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yamlDocumentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +6
-0@@ -0,0 +1,146 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/melexis,mlx7502x.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# +Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml line 16
Quoted 10 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +10
-0+title: Melexis ToF 7502x MIPI CSI-2 Sensor + +maintainers: + - Volodymyr Kharuk <[email protected]> + +description: |- + Melexis ToF 7502x sensors has a CSI-2 output. It supports 2 and 4 lanes, + and mipi speeds are 300, 600, 704, 800, 904, 960Mbs. Supported format is RAW12. + Sensor 75026 is QVGA, while 75027 is VGA sensor. + If you use compatible = "melexis,mlx7502x", then autodetect will be called.I'd move this last line as a description of the compatible property, but I'm also not sure this should be mentioned in the DT bindings, as it's a driver implementation detail. I'm actually not sure we should support it with three different compatible values as proposed, as without this documentation users will have a hard time figuring out what compatible value to pick.
One option would be to support the following three compatible values:
compatible = "melexis,mlx75026", "melexis,mlx7502x"; compatible = "melexis,mlx75027", "melexis,mlx7502x"; compatible = "melexis,mlx7502x";The last one only would trigger autodetection. I'm still not sure how to document that properly in bindings though.
Quoted 6 lines
Quoted 3 lines
I missed that part of binding.
Wildcards are not allowed in compatible, so mlx7502x has to go.
Really ? We've had fallback generic compatible strings since the beginning.
Fallback generic compatibles are allowed. Wildcards not. Wildcards were actually never explicitly allowed, they just slipped in to many bindings... We have several discussions on this on mailing list, so no real point to repeat the arguments.
There is a difference between generic fallback. If the device follows clear specification and version, e.g. "foo-bar-v4", you can use it for generic compatible. This is more common in SoC components. Requirement - there is a clear mapping between versions and SoCs.
Quoted 5 lines
Quoted 2 lines
Anyway what does this autodetection mean?
As far as I understand, it means that the driver will use a hardware identification register to figure out if the sensor is a 75026 or 75027.
Then there is no need to define 75027 compatible. DT is for cases where autodetection does not work...
Quoted 8 lines
The upside is that one doesn't need to change the device tree when swapping between those two sensors. The downside is that the sensor needs to be powered up at probe time. Depending on the platform, one of those two behaviours is preferred. Auto-detection is nice, but in laptops or tablets (not a use case for this particular device, but the problem applies to camera sensors in general), it would mean that the privacy LED of the camera could be briefly lit at boot time due to the sensor being powered on, which can worry users.
OK, that's reasonable argument for dedicated compatible but I don't understand why you cannot perform autodetection the moment device is actually powered up (first time). I understand it is nice and easy to make everything in the probe and most devices perform it that way. But if you don't want to do it in the probe - DT is not a workaround for this...
Best regards,
Krzysztof
Re: [PATCH v2 5/6] media: dt-bindings: media: i2c: Add mlx7502x camera sensor binding
Quoted 60 lines · Krzysztof Kozlowski
Quoted 59 lines · Laurent Pinchart
Quoted 57 lines · Krzysztof Kozlowski
Quoted 44 lines · Laurent Pinchart
Hi Volodymyr,
Thank you for the patch.
Quoted 21 lines · Volodymyr Kharuk
Add device tree binding of the mlx7502x and update
MAINTAINERSSigned-off-by Volodymyr Kharuk <[email protected]>--- .../bindings/media/i2c/melexis,mlx7502x.yaml | 146 ++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 147 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yamlDocumentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +6
-0@@ -0,0 +1,146 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/melexis,mlx7502x.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# +Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml line 16
Quoted 10 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +10
-0+title: Melexis ToF 7502x MIPI CSI-2 Sensor + +maintainers: + - Volodymyr Kharuk <[email protected]> + +description: |- + Melexis ToF 7502x sensors has a CSI-2 output. It supports 2 and 4 lanes, + and mipi speeds are 300, 600, 704, 800, 904, 960Mbs. Supported format is RAW12. + Sensor 75026 is QVGA, while 75027 is VGA sensor. + If you use compatible = "melexis,mlx7502x", then autodetect will be called.I'd move this last line as a description of the compatible property, but I'm also not sure this should be mentioned in the DT bindings, as it's a driver implementation detail. I'm actually not sure we should support it with three different compatible values as proposed, as without this documentation users will have a hard time figuring out what compatible value to pick.
Quoted 8 lines
One option would be to support the following three compatible values:
compatible = "melexis,mlx75026", "melexis,mlx7502x"; compatible = "melexis,mlx75027", "melexis,mlx7502x"; compatible = "melexis,mlx7502x";The last one only would trigger autodetection. I'm still not sure how to document that properly in bindings though.
I missed that part of binding.
Wildcards are not allowed in compatible, so mlx7502x has to go.
Quoted 7 lines
Quoted 2 lines
Really ? We've had fallback generic compatible strings since the beginning.
Fallback generic compatibles are allowed. Wildcards not. Wildcards were actually never explicitly allowed, they just slipped in to many bindings... We have several discussions on this on mailing list, so no real point to repeat the arguments.
Although I forgot one more acceptable case - family of devices followed by a specific compatible. However that "family" cannot be on its own.
Best regards,
Krzysztof
Laurent Pinchart
maintainerRe: [PATCH v2 5/6] media: dt-bindings: media: i2c: Add mlx7502x camera sensor binding
In reply to Krzysztof KozlowskiQuoted 68 lines · Krzysztof Kozlowski
Quoted 55 lines · Laurent Pinchart
Quoted 54 lines · Krzysztof Kozlowski
Quoted 52 lines · Laurent Pinchart
Hi Volodymyr,
Thank you for the patch.
Quoted 21 lines · Volodymyr Kharuk
Add device tree binding of the mlx7502x and update
MAINTAINERSSigned-off-by Volodymyr Kharuk <[email protected]>--- .../bindings/media/i2c/melexis,mlx7502x.yaml | 146 ++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 147 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yamlDocumentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +6
-0@@ -0,0 +1,146 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/melexis,mlx7502x.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# +Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml line 16
Quoted 10 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +10
-0+title: Melexis ToF 7502x MIPI CSI-2 Sensor + +maintainers: + - Volodymyr Kharuk <[email protected]> + +description: |- + Melexis ToF 7502x sensors has a CSI-2 output. It supports 2 and 4 lanes, + and mipi speeds are 300, 600, 704, 800, 904, 960Mbs. Supported format is RAW12. + Sensor 75026 is QVGA, while 75027 is VGA sensor. + If you use compatible = "melexis,mlx7502x", then autodetect will be called.I'd move this last line as a description of the compatible property, but I'm also not sure this should be mentioned in the DT bindings, as it's a driver implementation detail. I'm actually not sure we should support it with three different compatible values as proposed, as without this documentation users will have a hard time figuring out what compatible value to pick.
One option would be to support the following three compatible values:
compatible = "melexis,mlx75026", "melexis,mlx7502x"; compatible = "melexis,mlx75027", "melexis,mlx7502x"; compatible = "melexis,mlx7502x";The last one only would trigger autodetection. I'm still not sure how to document that properly in bindings though.
Quoted 6 lines
Quoted 3 lines
I missed that part of binding.
Wildcards are not allowed in compatible, so mlx7502x has to go.
Really ? We've had fallback generic compatible strings since the beginning.
Fallback generic compatibles are allowed. Wildcards not. Wildcards were actually never explicitly allowed, they just slipped in to many bindings... We have several discussions on this on mailing list, so no real point to repeat the arguments.
Quoted 4 lines
There is a difference between generic fallback. If the device follows clear specification and version, e.g. "foo-bar-v4", you can use it for generic compatible. This is more common in SoC components. Requirement - there is a clear mapping between versions and SoCs.
I'm not sure to see a clear difference between the two concepts.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Anyway what does this autodetection mean?
As far as I understand, it means that the driver will use a hardware identification register to figure out if the sensor is a 75026 or 75027.
Then there is no need to define 75027 compatible. DT is for cases where autodetection does not work...
It's autodetection of the exact device model, those are I2C devices so we still need DT, and we still need to know that it's one of the MLX75026 or MLX75027.
Quoted 9 lines
Quoted 8 lines
The upside is that one doesn't need to change the device tree when swapping between those two sensors. The downside is that the sensor needs to be powered up at probe time. Depending on the platform, one of those two behaviours is preferred. Auto-detection is nice, but in laptops or tablets (not a use case for this particular device, but the problem applies to camera sensors in general), it would mean that the privacy LED of the camera could be briefly lit at boot time due to the sensor being powered on, which can worry users.
Quoted 5 lines
OK, that's reasonable argument for dedicated compatible but I don't understand why you cannot perform autodetection the moment device is actually powered up (first time). I understand it is nice and easy to make everything in the probe and most devices perform it that way. But if you don't want to do it in the probe - DT is not a workaround for this...
For cameras, we often deal with complex pipelines with multiple external devices and multiple IP cores, with drivers that need to communicate with each other to initialize the complete camera system. For instance, each camera-related component in the system registers itself in a media graph that can be queried from userspace and exposes information about all devices, including their model. There's no power up of any device when this query is being performed from userspace. It could possibly be changed (and maybe it should, for reasons unrelated to this discussion), but we're looking at pretty much a complete redesign of V4L2 and MC then.
Signature · 3 lines
Regards, Laurent Pinchart
Re: [PATCH v2 5/6] media: dt-bindings: media: i2c: Add mlx7502x camera sensor binding
Quoted 21 lines · Laurent Pinchart
Quoted 21 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
One option would be to support the following three compatible values:
compatible = "melexis,mlx75026", "melexis,mlx7502x"; compatible = "melexis,mlx75027", "melexis,mlx7502x"; compatible = "melexis,mlx7502x";The last one only would trigger autodetection. I'm still not sure how to document that properly in bindings though.
Quoted 6 lines
Quoted 3 lines
I missed that part of binding.
Wildcards are not allowed in compatible, so mlx7502x has to go.
Really ? We've had fallback generic compatible strings since the beginning.
Fallback generic compatibles are allowed. Wildcards not. Wildcards were actually never explicitly allowed, they just slipped in to many bindings... We have several discussions on this on mailing list, so no real point to repeat the arguments.
Quoted 6 lines
Quoted 4 lines
There is a difference between generic fallback. If the device follows clear specification and version, e.g. "foo-bar-v4", you can use it for generic compatible. This is more common in SoC components. Requirement - there is a clear mapping between versions and SoCs.
I'm not sure to see a clear difference between the two concepts.
The clear difference is that you have a versioned and re-usable hardware block plus clear mapping which version goes to which SoC. Version numbers usually start with 1, not with 75025. 75025 is a model name.
Quoted 27 lines
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Anyway what does this autodetection mean?
As far as I understand, it means that the driver will use a hardware identification register to figure out if the sensor is a 75026 or 75027.
Then there is no need to define 75027 compatible. DT is for cases where autodetection does not work...
It's autodetection of the exact device model, those are I2C devices so we still need DT, and we still need to know that it's one of the MLX75026 or MLX75027.
Quoted 9 lines
Quoted 8 lines
The upside is that one doesn't need to change the device tree when swapping between those two sensors. The downside is that the sensor needs to be powered up at probe time. Depending on the platform, one of those two behaviours is preferred. Auto-detection is nice, but in laptops or tablets (not a use case for this particular device, but the problem applies to camera sensors in general), it would mean that the privacy LED of the camera could be briefly lit at boot time due to the sensor being powered on, which can worry users.
Quoted 5 lines
OK, that's reasonable argument for dedicated compatible but I don't understand why you cannot perform autodetection the moment device is actually powered up (first time). I understand it is nice and easy to make everything in the probe and most devices perform it that way. But if you don't want to do it in the probe - DT is not a workaround for this...
Quoted 10 lines
For cameras, we often deal with complex pipelines with multiple external devices and multiple IP cores, with drivers that need to communicate with each other to initialize the complete camera system. For instance, each camera-related component in the system registers itself in a media graph that can be queried from userspace and exposes information about all devices, including their model. There's no power up of any device when this query is being performed from userspace. It could possibly be changed (and maybe it should, for reasons unrelated to this discussion), but we're looking at pretty much a complete redesign of V4L2 and MC then.
Is then autodetection a real use case since you have to power up the sensor each time system boots and this violates privacy? Several I2C sensors do not care about this and they always do it on power up, so aren't we solving here something unimportant?
Best regards,
Krzysztof
Laurent Pinchart
maintainerRe: [PATCH v2 5/6] media: dt-bindings: media: i2c: Add mlx7502x camera sensor binding
Quoted 27 lines · Krzysztof Kozlowski
Quoted 26 lines · Laurent Pinchart
Quoted 25 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
One option would be to support the following three compatible values:
compatible = "melexis,mlx75026", "melexis,mlx7502x"; compatible = "melexis,mlx75027", "melexis,mlx7502x"; compatible = "melexis,mlx7502x";The last one only would trigger autodetection. I'm still not sure how to document that properly in bindings though.
Quoted 6 lines
Quoted 3 lines
I missed that part of binding.
Wildcards are not allowed in compatible, so mlx7502x has to go.
Really ? We've had fallback generic compatible strings since the beginning.
Fallback generic compatibles are allowed. Wildcards not. Wildcards were actually never explicitly allowed, they just slipped in to many bindings... We have several discussions on this on mailing list, so no real point to repeat the arguments.
There is a difference between generic fallback. If the device follows clear specification and version, e.g. "foo-bar-v4", you can use it for generic compatible. This is more common in SoC components. Requirement - there is a clear mapping between versions and SoCs.
Quoted 5 lines
Quoted 1 line
I'm not sure to see a clear difference between the two concepts.
The clear difference is that you have a versioned and re-usable hardware block plus clear mapping which version goes to which SoC. Version numbers usually start with 1, not with 75025. 75025 is a model name.
How about Documentation/devicetree/bindings/serial/renesas,scif.yaml for instance, where the version number isn't known and the SoC name is used instead ? Is that acceptable ?
How should we deal with devices that have different models, where the model is irrelevant to the kernel driver, but relevant to userspace ? Imagine, for instance, a light sensor with 10 models than only differ by the filter they use to tune the sensitivity to different light spectrums ? They are all "compatible" from a software point of view, would the driver need to list all 10 compatible strings ?
Quoted 38 lines
Quoted 27 lines
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Anyway what does this autodetection mean?
As far as I understand, it means that the driver will use a hardware identification register to figure out if the sensor is a 75026 or 75027.
Then there is no need to define 75027 compatible. DT is for cases where autodetection does not work...
It's autodetection of the exact device model, those are I2C devices so we still need DT, and we still need to know that it's one of the MLX75026 or MLX75027.
Quoted 9 lines
Quoted 8 lines
The upside is that one doesn't need to change the device tree when swapping between those two sensors. The downside is that the sensor needs to be powered up at probe time. Depending on the platform, one of those two behaviours is preferred. Auto-detection is nice, but in laptops or tablets (not a use case for this particular device, but the problem applies to camera sensors in general), it would mean that the privacy LED of the camera could be briefly lit at boot time due to the sensor being powered on, which can worry users.
Quoted 5 lines
OK, that's reasonable argument for dedicated compatible but I don't understand why you cannot perform autodetection the moment device is actually powered up (first time). I understand it is nice and easy to make everything in the probe and most devices perform it that way. But if you don't want to do it in the probe - DT is not a workaround for this...
Quoted 10 lines
For cameras, we often deal with complex pipelines with multiple external devices and multiple IP cores, with drivers that need to communicate with each other to initialize the complete camera system. For instance, each camera-related component in the system registers itself in a media graph that can be queried from userspace and exposes information about all devices, including their model. There's no power up of any device when this query is being performed from userspace. It could possibly be changed (and maybe it should, for reasons unrelated to this discussion), but we're looking at pretty much a complete redesign of V4L2 and MC then.
Quoted 4 lines
Is then autodetection a real use case since you have to power up the sensor each time system boots and this violates privacy? Several I2C sensors do not care about this and they always do it on power up, so aren't we solving here something unimportant?
In a laptop or tablet with a camera sensor, you likely don't want autodetection. In an industrial device, you don't care, and having the ability to auto-detect the exact sensor model when booting saves cost in the production chain as a single image can work across different models.
Signature · 3 lines
Regards, Laurent Pinchart
Re: [PATCH v2 5/6] media: dt-bindings: media: i2c: Add mlx7502x camera sensor binding
Quoted 30 lines · Laurent Pinchart
Quoted 29 lines · Krzysztof Kozlowski
Quoted 27 lines · Laurent Pinchart
Quoted 21 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
One option would be to support the following three compatible values:
compatible = "melexis,mlx75026", "melexis,mlx7502x"; compatible = "melexis,mlx75027", "melexis,mlx7502x"; compatible = "melexis,mlx7502x";The last one only would trigger autodetection. I'm still not sure how to document that properly in bindings though.
Quoted 6 lines
Quoted 3 lines
I missed that part of binding.
Wildcards are not allowed in compatible, so mlx7502x has to go.
Really ? We've had fallback generic compatible strings since the beginning.
Fallback generic compatibles are allowed. Wildcards not. Wildcards were actually never explicitly allowed, they just slipped in to many bindings... We have several discussions on this on mailing list, so no real point to repeat the arguments.
Quoted 4 lines
There is a difference between generic fallback. If the device follows clear specification and version, e.g. "foo-bar-v4", you can use it for generic compatible. This is more common in SoC components. Requirement - there is a clear mapping between versions and SoCs.
I'm not sure to see a clear difference between the two concepts.
Quoted 7 lines
Quoted 3 lines
The clear difference is that you have a versioned and re-usable hardware block plus clear mapping which version goes to which SoC. Version numbers usually start with 1, not with 75025. 75025 is a model name.
How about
Documentation/devicetree/bindings/serial/renesas,scif.yamlfor instance, where the version number isn't known and the SoC name is used instead ? Is that acceptable ?
This is the second case I mentioned - family of devices where the family fallback is not allowed to be alone. You cannot use just "renesas,scif" in DTS.
Quoted 7 lines
How should we deal with devices that have different models, where the model is irrelevant to the kernel driver, but relevant to userspace ? Imagine, for instance, a light sensor with 10 models than only differ by the filter they use to tune the sensitivity to different light spectrums ? They are all "compatible" from a software point of view, would the driver need to list all 10 compatible strings ?
I don't understand that example, I mean, what's the problem here? If they are all compatible, you can use only one comaptible, e.g. melexis,mlx75026.
If you ever need to differentiate it for user-space, you add specific compatible for the model and you have:
melexis,mlx75027, melexis,mlx75026
If user-space needs dedicated compatibles - add them, no one here argues to not to use specific compatibles.
Quoted 16 lines
Quoted 11 lines
Quoted 10 lines
For cameras, we often deal with complex pipelines with multiple external devices and multiple IP cores, with drivers that need to communicate with each other to initialize the complete camera system. For instance, each camera-related component in the system registers itself in a media graph that can be queried from userspace and exposes information about all devices, including their model. There's no power up of any device when this query is being performed from userspace. It could possibly be changed (and maybe it should, for reasons unrelated to this discussion), but we're looking at pretty much a complete redesign of V4L2 and MC then.
Quoted 4 lines
Is then autodetection a real use case since you have to power up the sensor each time system boots and this violates privacy? Several I2C sensors do not care about this and they always do it on power up, so aren't we solving here something unimportant?
Quoted 4 lines
In a laptop or tablet with a camera sensor, you likely don't want autodetection. In an industrial device, you don't care, and having the ability to auto-detect the exact sensor model when booting saves cost in the production chain as a single image can work across different models.
We talk about the case here, not generic. Do you want to have autodetection possible here or not?
Best regards,
Krzysztof
Re: [PATCH v2 4/6] media: uapi: Add mlx7502x header file
In reply to Laurent PinchartHi Laurent,
Thank you for your review.
Quoted 46 lines · Laurent Pinchart
Hi Volodymyr,
Thank you for the patch.
Quoted 41 lines · Volodymyr Kharuk
Define user controls for mlx7502x driver and update
MAINTAINERSSigned-off-by Volodymyr Kharuk <[email protected]>--- MAINTAINERS | 7 +++++++ include/uapi/linux/mlx7502x.h | 31 +++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100644 include/uapi/linux/mlx7502x.hMAINTAINERS +7
-0@@ -12673,6 +12673,13 @@ S: Supported W: http://www.melexis.com F: drivers/iio/temperature/mlx90632.c +MELEXIS MLX7502X DRIVER +M: Volodymyr Kharuk <[email protected]> +L: [email protected] +S: Supported +W: http://www.melexis.com +F: include/uapi/linux/mlx7502x.h + MELFAS MIP4 TOUCHSCREEN DRIVER M: Sangwon Jee <[email protected]> S: Supportedinclude/uapi/linux/mlx7502x.h new file +8
-0@@ -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. +* +*/ +
Quoted 8 lines
include/uapi/linux/mlx7502x.h line 13
Quoted 5 lines
include/uapi/linux/mlx7502x.h new file +5
-0+#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
Quoted 12 lines
include/uapi/linux/mlx7502x.h line 21
Quoted 9 lines
include/uapi/linux/mlx7502x.h new file +8
-0+/* 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?
Quoted 8 lines
include/uapi/linux/mlx7502x.h line 25
Quoted 5 lines
include/uapi/linux/mlx7502x.h new file +4
-0+/* 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.
Quoted 8 lines
include/uapi/linux/mlx7502x.h line 29
Quoted 5 lines
include/uapi/linux/mlx7502x.h new file +4
-0+/* 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/[email protected]/
Ok, then I'll remove these controls for now.
Quoted 8 lines
Quoted 3 lines
include/uapi/linux/mlx7502x.h new file +2
-0+ +#endif /* __UAPI_MLX7502X_H_ */ Signature · 3 lines
Regards, Laurent Pinchart
Signature · 2 lines
-- Volodymyr Kharuk
Laurent Pinchart
maintainerRe: [PATCH v2 4/6] media: uapi: Add mlx7502x header file
Hello,
CC'ing Benjamin Mugnier who I recall expressed an interest in ToF sensors (if I recall incorrectly, my apologies).
Quoted 66 lines · Volodymyr Kharuk
Quoted 42 lines · Laurent Pinchart
Quoted 41 lines · Volodymyr Kharuk
Define user controls for mlx7502x driver and update
MAINTAINERSSigned-off-by Volodymyr Kharuk <[email protected]>--- MAINTAINERS | 7 +++++++ include/uapi/linux/mlx7502x.h | 31 +++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100644 include/uapi/linux/mlx7502x.hMAINTAINERS +7
-0@@ -12673,6 +12673,13 @@ S: Supported W: http://www.melexis.com F: drivers/iio/temperature/mlx90632.c +MELEXIS MLX7502X DRIVER +M: Volodymyr Kharuk <[email protected]> +L: [email protected] +S: Supported +W: http://www.melexis.com +F: include/uapi/linux/mlx7502x.h + MELFAS MIP4 TOUCHSCREEN DRIVER M: Sangwon Jee <[email protected]> S: Supportedinclude/uapi/linux/mlx7502x.h new file +8
-0@@ -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. +* +*/ +Quoted 8 lines
include/uapi/linux/mlx7502x.h line 13
Quoted 5 lines
include/uapi/linux/mlx7502x.h new file +5
-0+#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
Quoted 11 lines
include/uapi/linux/mlx7502x.h line 21
Quoted 8 lines
include/uapi/linux/mlx7502x.h new file +8
-0+/* 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.
Quoted 2 lines
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.
Quoted 3 lines
Note that the control
V4L2_CID_MLX7502X_TINTis similar toV4L2_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.
Quoted 23 lines
Quoted 7 lines
include/uapi/linux/mlx7502x.h line 25
Quoted 4 lines
include/uapi/linux/mlx7502x.h new file +4
-0+/* 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.
Quoted 7 lines
include/uapi/linux/mlx7502x.h line 29
Quoted 4 lines
include/uapi/linux/mlx7502x.h new file +4
-0+/* 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/[email protected]/Ok, then I'll remove these controls for now.
Quoted 2 lines
Quoted 2 lines
include/uapi/linux/mlx7502x.h new file +2
-0+ +#endif /* __UAPI_MLX7502X_H_ */
Signature · 3 lines
Regards, Laurent Pinchart
Re: [PATCH v2 4/6] media: uapi: Add mlx7502x header file
Quoted 72 lines · Laurent Pinchart
Hello,
CC'ing Benjamin Mugnier who I recall expressed an interest in ToF sensors (if I recall incorrectly, my apologies).
Quoted 66 lines · Volodymyr Kharuk
Quoted 42 lines · Laurent Pinchart
Quoted 41 lines · Volodymyr Kharuk
Define user controls for mlx7502x driver and update
MAINTAINERSSigned-off-by Volodymyr Kharuk <[email protected]>--- MAINTAINERS | 7 +++++++ include/uapi/linux/mlx7502x.h | 31 +++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100644 include/uapi/linux/mlx7502x.hMAINTAINERS +7
-0@@ -12673,6 +12673,13 @@ S: Supported W: http://www.melexis.com F: drivers/iio/temperature/mlx90632.c +MELEXIS MLX7502X DRIVER +M: Volodymyr Kharuk <[email protected]> +L: [email protected] +S: Supported +W: http://www.melexis.com +F: include/uapi/linux/mlx7502x.h + MELFAS MIP4 TOUCHSCREEN DRIVER M: Sangwon Jee <[email protected]> S: Supportedinclude/uapi/linux/mlx7502x.h new file +8
-0@@ -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. +* +*/ +Quoted 8 lines
include/uapi/linux/mlx7502x.h line 13
Quoted 5 lines
include/uapi/linux/mlx7502x.h new file +5
-0+#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
Quoted 11 lines
include/uapi/linux/mlx7502x.h line 21
Quoted 8 lines
include/uapi/linux/mlx7502x.h new file +8
-0+/* 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.
Quoted 5 lines
Quoted 2 lines
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 :)
Quoted 5 lines
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.
Quoted 35 lines
Quoted 4 lines
Note that the control
V4L2_CID_MLX7502X_TINTis similar toV4L2_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.
Quoted 23 lines
Quoted 7 lines
include/uapi/linux/mlx7502x.h line 25
Quoted 4 lines
include/uapi/linux/mlx7502x.h new file +4
-0+/* 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.
Quoted 7 lines
include/uapi/linux/mlx7502x.h line 29
Quoted 4 lines
include/uapi/linux/mlx7502x.h new file +4
-0+/* 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/[email protected]/Ok, then I'll remove these controls for now.
Quoted 2 lines
Quoted 2 lines
include/uapi/linux/mlx7502x.h new file +2
-0+ +#endif /* __UAPI_MLX7502X_H_ */ Signature · 3 lines
Regards, Laurent Pinchart
Signature · 2 lines
-- Volodymyr Kharuk
Re: [PATCH v2 5/6] media: dt-bindings: media: i2c: Add mlx7502x camera sensor binding
In reply to Laurent PinchartHi Laurent,
Thank you for your review,
Quoted 104 lines · Laurent Pinchart
Hi Volodymyr,
Thank you for the patch.
Quoted 21 lines · Volodymyr Kharuk
Add device tree binding of the mlx7502x and update
MAINTAINERSSigned-off-by Volodymyr Kharuk <[email protected]>--- .../bindings/media/i2c/melexis,mlx7502x.yaml | 146 ++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 147 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yamlDocumentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +6
-0@@ -0,0 +1,146 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/melexis,mlx7502x.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# +Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml line 16
Quoted 10 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +10
-0+title: Melexis ToF 7502x MIPI CSI-2 Sensor + +maintainers: + - Volodymyr Kharuk <[email protected]> + +description: |- + Melexis ToF 7502x sensors has a CSI-2 output. It supports 2 and 4 lanes, + and mipi speeds are 300, 600, 704, 800, 904, 960Mbs. Supported format is RAW12. + Sensor 75026 is QVGA, while 75027 is VGA sensor. + If you use compatible = "melexis,mlx7502x", then autodetect will be called.I'd move this last line as a description of the compatible property, but I'm also not sure this should be mentioned in the DT bindings, as it's a driver implementation detail. I'm actually not sure we should support it with three different compatible values as proposed, as without this documentation users will have a hard time figuring out what compatible value to pick.
One option would be to support the following three compatible values:
compatible = "melexis,mlx75026", "melexis,mlx7502x"; compatible = "melexis,mlx75027", "melexis,mlx7502x"; compatible = "melexis,mlx7502x";The last one only would trigger autodetection. I'm still not sure how to document that properly in bindings though.
Quoted 51 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +51
-0+ +properties: + compatible: + items: + - enum: + - melexis,mlx7502x + - melexis,mlx75026 + - melexis,mlx75027 + + reg: + maxItems: 1 + + assigned-clocks: true + assigned-clock-parents: true + assigned-clock-rates: true + + clocks: + description: Clock frequency 8MHz + maxItems: 1 + + vdda-supply: + description: + Definition of the regulator used as analog power supply(2.7V). + + vddif-supply: + description: + Definition of the regulator used as interface power supply(1.8V). + + vddd-supply: + description: + Definition of the regulator used as digital power supply(1.2V). + + vdmix-supply: + description: + Definition of the regulator used as mixed driver power supply(1.2V). + + reset-gpios: + maxItems: 1 + description: Reset Sensor GPIO Control (active low) + + melexis,trig-gpios: + maxItems: 1 + description: + Hardware Trigger GPIO Control (active low) + When the hardware trigger mode is enabled, this GPIO is used to generate + a low level impulse for about 100us. The impulse triggers a new + measurement cycle. + + melexis,leden-gpios: + maxItems: 1 + description:
Quoted 8 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml line 70
Quoted 3 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +3
-0+ Led driver enable GPIO Control (active high) + This GPIO notifies the illumination driver when it is safe to use led + signals from the sensor.As far as I understand this isn't an input of the sensor, but a signal that is driven by the driver and controls the separate illuminator. It shouldn't be specified in this binding, as it's not a property of the sensor. You should instead have a DT node for the illumination driver.
Yes, you are right. There are illuminators, which are not ready to accept the signal from the sensor, if pin levels are not right. So I need to notify illuminator somehow. Can the illumination driver be as a V4L2 subdevice? Then I can notify the illuminator via s_stream. In another case it can damage the illuminator. What do you think?
Quoted 29 lines
Quoted 29 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +28
-0+ port: + description: MIPI CSI-2 transmitter port + $ref: /schemas/graph.yaml#/$defs/port-base + + properties: + endpoint: + $ref: /schemas/media/video-interfaces.yaml# + unevaluatedProperties: false + + properties: + data-lanes: + oneOf: + - items: + - const: 1 + - const: 2 + - items: + - const: 1 + - const: 2 + - const: 3 + - const: 4 + + clock-noncontinuous: true + link-frequencies: true + + required: + - data-lanes + - link-frequencies +
Quoted 8 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml line 104
Quoted 5 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +5
-0+required: + - compatible + - reg + - clocks + - portLet's make the supplies mandatory too, as the chip can't operate without them.
Ok
Quoted 60 lines
Quoted 55 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +42
-0+ +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + + i2c { + #address-cells = <1>; + #size-cells = <0>; + + mlx7502x: camera@57 { + compatible = "melexis,mlx7502x"; + reg = <0x57>; + clocks = <&mlx7502x_clk>; + + assigned-clocks = <&mlx7502x_clk>; + assigned-clock-parents = <&mlx7502x_clk_parent>; + assigned-clock-rates = <8000000>; + + vddd-supply = <&mlx_7502x_reg>; + + reset-gpios = <&gpio_exp 6 GPIO_ACTIVE_HIGH>; + melexis,trig-gpios = <&gpio_exp 2 GPIO_ACTIVE_HIGH>; + melexis,leden-gpios = <&gpio_exp 7 GPIO_ACTIVE_HIGH>; + + port { + mlx7502x_out_mipi_csi2: endpoint { + remote-endpoint = <&mipi_csi2_from_mlx7502x>; + data-lanes = <1 2 3 4>; + link-frequencies = /bits/ 64 < 960000000 + 904000000 + 800000000 + 704000000 + 600000000 + 300000000 >; + }; + }; + }; + }; + +...MAINTAINERS +1
-0@@ -12678,6 +12678,7 @@ M: Volodymyr Kharuk <[email protected]> L: [email protected] S: Supported W: http://www.melexis.com +F: Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml F: include/uapi/linux/mlx7502x.h MELFAS MIP4 TOUCHSCREEN DRIVERSignature · 3 lines
Regards, Laurent Pinchart
Signature · 2 lines
-- Volodymyr Kharuk
kernel test robot
build botRe: [PATCH v2 6/6] media: i2c: Add driver for mlx7502x ToF sensor
In reply to Volodymyr KharukI love your patch! Perhaps something to improve:
Hi Volodymyr,
I love your patch! Perhaps something to improve:
[auto build test WARNING on d8e8aa866ed8636fd6c1017c3d9453eab2922496]
url: https://github.com/intel-lab-lkp/linux/commits/Volodymyr-Kharuk/media-i2c-mlx7502x-ToF-camera-support/20220714-163709 base: d8e8aa866ed8636fd6c1017c3d9453eab2922496 config: parisc-randconfig-s041-20220715 (https://download.01.org/0day-ci/archive/20220717/[email protected]/config) compiler: hppa64-linux-gcc (GCC) 12.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/59a691c2f449518699a328ea663098aa3ae038b9
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Volodymyr-Kharuk/media-i2c-mlx7502x-ToF-camera-support/20220714-163709
git checkout 59a691c2f449518699a328ea663098aa3ae038b9
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=parisc64 SHELL=/bin/bash drivers/media/i2c/ drivers/pci/ kernel/trace/ sound/soc/codecs/
If you fix the issue, kindly add following tag where applicable
sparse warnings: (new ones prefixed by >>)
>> drivers/media/i2c/mlx7502x.c:1989:53: sparse: sparse: Using plain integer as NULL pointer
vim +1989 drivers/media/i2c/mlx7502x.c
1987
1988 static const struct of_device_id mlx7502x_of_match[] = {
> 1989 { .compatible = "melexis,mlx7502x", .data = 0 }, /* auto-detection */ 1990 { .compatible = "melexis,mlx75026", .data = &mlx75026 },
1991 { .compatible = "melexis,mlx75027", .data = &mlx75027 },
1992 { /* sentinel */ }
1993 };
1994 MODULE_DEVICE_TABLE(of, mlx7502x_of_match);
1995
Signature · 2 lines
0-DAY CI Kernel Test Service https://01.org/lkp
kernel test robot
build botRe: [PATCH v2 6/6] media: i2c: Add driver for mlx7502x ToF sensor
In reply to Volodymyr KharukI love your patch! Perhaps something to improve:
Hi Volodymyr,
I love your patch! Perhaps something to improve:
[auto build test WARNING on d8e8aa866ed8636fd6c1017c3d9453eab2922496]
url: https://github.com/intel-lab-lkp/linux/commits/Volodymyr-Kharuk/media-i2c-mlx7502x-ToF-camera-support/20220714-163709 base: d8e8aa866ed8636fd6c1017c3d9453eab2922496 config: parisc-randconfig-s041-20220715 (https://download.01.org/0day-ci/archive/20220718/[email protected]/config) compiler: hppa64-linux-gcc (GCC) 12.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/59a691c2f449518699a328ea663098aa3ae038b9
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Volodymyr-Kharuk/media-i2c-mlx7502x-ToF-camera-support/20220714-163709
git checkout 59a691c2f449518699a328ea663098aa3ae038b9
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=parisc64 SHELL=/bin/bash drivers/media/i2c/ drivers/pci/ kernel/trace/ sound/soc/codecs/
If you fix the issue, kindly add following tag where applicable
sparse warnings: (new ones prefixed by >>)
>> drivers/media/i2c/mlx7502x.c:1989:53: sparse: sparse: Using plain integer as NULL pointer
vim +1989 drivers/media/i2c/mlx7502x.c
1987
1988 static const struct of_device_id mlx7502x_of_match[] = {
> 1989 { .compatible = "melexis,mlx7502x", .data = 0 }, /* auto-detection */ 1990 { .compatible = "melexis,mlx75026", .data = &mlx75026 },
1991 { .compatible = "melexis,mlx75027", .data = &mlx75027 },
1992 { /* sentinel */ }
1993 };
1994 MODULE_DEVICE_TABLE(of, mlx7502x_of_match);
1995
Signature · 2 lines
0-DAY CI Kernel Test Service https://01.org/lkp
kernel test robot
build botRe: [PATCH v2 6/6] media: i2c: Add driver for mlx7502x ToF sensor
In reply to Volodymyr KharukI love your patch! Perhaps something to improve:
Hi Volodymyr,
I love your patch! Perhaps something to improve:
[auto build test WARNING on d8e8aa866ed8636fd6c1017c3d9453eab2922496]
url: https://github.com/intel-lab-lkp/linux/commits/Volodymyr-Kharuk/media-i2c-mlx7502x-ToF-camera-support/20220714-163709 base: d8e8aa866ed8636fd6c1017c3d9453eab2922496 config: hexagon-allyesconfig (https://download.01.org/0day-ci/archive/20220718/[email protected]/config) compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 07022e6cf9b5b3baa642be53d0b3c3f1c403dbfd)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/59a691c2f449518699a328ea663098aa3ae038b9
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Volodymyr-Kharuk/media-i2c-mlx7502x-ToF-camera-support/20220714-163709
git checkout 59a691c2f449518699a328ea663098aa3ae038b9
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/media/i2c/ drivers/net/pcs/ kernel/trace/
If you fix the issue, kindly add following tag where applicable
All warnings (new ones prefixed by >>):
>> drivers/media/i2c/mlx7502x.c:981:13: warning: variable 'ret' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
} else if (!on && sensor->streaming) {
^~~~~~~~~~~~~~~~~~~~~~~~
drivers/media/i2c/mlx7502x.c:993:9: note: uninitialized use occurs here
return ret;
^~~
drivers/media/i2c/mlx7502x.c:981:9: note: remove the 'if' if its condition is always true
} else if (!on && sensor->streaming) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/media/i2c/mlx7502x.c:981:13: warning: variable 'ret' is used uninitialized whenever '&&' condition is false [-Wsometimes-uninitialized]
} else if (!on && sensor->streaming) {
^~~
drivers/media/i2c/mlx7502x.c:993:9: note: uninitialized use occurs here
return ret;
^~~
drivers/media/i2c/mlx7502x.c:981:13: note: remove the '&&' if its condition is always true
} else if (!on && sensor->streaming) {
^~~~~~
drivers/media/i2c/mlx7502x.c:945:9: note: initialize the variable 'ret' to silence this warning
int ret;
^
= 02 warnings generated.
vim +981 drivers/media/i2c/mlx7502x.c
941
942 static int mlx7502x_s_stream(struct v4l2_subdev *sd, int on)
943 {
944 struct mlx7502x *sensor = to_mlx7502x(sd);
945 int ret;
946
947 mutex_lock(&sensor->lock);
948
949 if (on && !sensor->streaming) {
950 ret = pm_runtime_resume_and_get(sensor->dev);
951 if (ret < 0) {
952 mutex_unlock(&sensor->lock);
953 return ret;
954 }
955
956 ret = __v4l2_ctrl_handler_setup(sd->ctrl_handler);
957 if (ret < 0)
958 goto error_stream;
959 /* the registers below depends on hmax, which is configured in controls */
960 ret = mlx7502x_set_link_freq(sensor);
961 if (ret < 0)
962 goto error_stream;
963 ret = mlx7502x_update_output_format(sensor);
964 if (ret < 0)
965 goto error_stream;
966 ret = mlx7502x_set_trigger_mode(sensor);
967 if (ret < 0)
968 goto error_stream;
969
970 ret = mlx7502x_write8(sd, MLX7502X_STREAM_EN_REG, 1u);
971 if (ret < 0)
972 goto error_stream;
973
974 sensor->streaming = 1u;
975 dev_dbg(sensor->dev, "stream enabled\n");
976
977 /* we need to wait to stabilize the system after streaming on */
978 usleep_range(MLX7502X_STREAMING_DELAY_US, MLX7502X_STREAMING_DELAY_US + 10);
979
980 gpiod_set_value_cansleep(sensor->leden, 1);
> 981 } else if (!on && sensor->streaming) { 982 gpiod_set_value_cansleep(sensor->leden, 0);
983
984 sensor->streaming = 0u;
985 ret = mlx7502x_write8(sd, MLX7502X_STREAM_EN_REG, 0u);
986 dev_dbg(sensor->dev, "stream disabled\n");
987
988 pm_runtime_mark_last_busy(sensor->dev);
989 pm_runtime_put_autosuspend(sensor->dev);
990 }
991
992 mutex_unlock(&sensor->lock);
993 return ret;
994
995 error_stream:
996 pm_runtime_put(sensor->dev);
997 mutex_unlock(&sensor->lock);
998 return ret;
999 }
1000
Signature · 2 lines
0-DAY CI Kernel Test Service https://01.org/lkp
kernel test robot
build botRe: [PATCH v2 6/6] media: i2c: Add driver for mlx7502x ToF sensor
In reply to Volodymyr KharukI love your patch! Perhaps something to improve:
Hi Volodymyr,
I love your patch! Perhaps something to improve:
[auto build test WARNING on d8e8aa866ed8636fd6c1017c3d9453eab2922496]
url: https://github.com/intel-lab-lkp/linux/commits/Volodymyr-Kharuk/media-i2c-mlx7502x-ToF-camera-support/20220714-163709 base: d8e8aa866ed8636fd6c1017c3d9453eab2922496 config: parisc-randconfig-s041-20220715 (https://download.01.org/0day-ci/archive/20220719/[email protected]/config) compiler: hppa64-linux-gcc (GCC) 12.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/59a691c2f449518699a328ea663098aa3ae038b9
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Volodymyr-Kharuk/media-i2c-mlx7502x-ToF-camera-support/20220714-163709
git checkout 59a691c2f449518699a328ea663098aa3ae038b9
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=parisc64 SHELL=/bin/bash drivers/media/i2c/ drivers/pci/ kernel/trace/ sound/soc/codecs/
If you fix the issue, kindly add following tag where applicable
sparse warnings: (new ones prefixed by >>)
>> drivers/media/i2c/mlx7502x.c:1989:53: sparse: sparse: Using plain integer as NULL pointer
vim +1989 drivers/media/i2c/mlx7502x.c
1987
1988 static const struct of_device_id mlx7502x_of_match[] = {
> 1989 { .compatible = "melexis,mlx7502x", .data = 0 }, /* auto-detection */ 1990 { .compatible = "melexis,mlx75026", .data = &mlx75026 },
1991 { .compatible = "melexis,mlx75027", .data = &mlx75027 },
1992 { /* sentinel */ }
1993 };
1994 MODULE_DEVICE_TABLE(of, mlx7502x_of_match);
1995
Signature · 2 lines
0-DAY CI Kernel Test Service https://01.org/lkp
Re: [PATCH v2 4/6] media: uapi: Add mlx7502x header file
In reply to Volodymyr KharukHi Volodymyr,
Quoted 5 lines · Volodymyr Kharuk
Quoted 4 lines · Laurent Pinchart
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 :)
Quoted 68 lines
Quoted 68 lines · Volodymyr Kharuk
Quoted 66 lines · Laurent Pinchart
Quoted 42 lines · Volodymyr Kharuk
Quoted 41 lines
Define user controls for mlx7502x driver and update
MAINTAINERSSigned-off-by Volodymyr Kharuk <[email protected]>--- MAINTAINERS | 7 +++++++ include/uapi/linux/mlx7502x.h | 31 +++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100644 include/uapi/linux/mlx7502x.hMAINTAINERS +7
-0@@ -12673,6 +12673,13 @@ S: Supported W: http://www.melexis.com F: drivers/iio/temperature/mlx90632.c +MELEXIS MLX7502X DRIVER +M: Volodymyr Kharuk <[email protected]> +L: [email protected] +S: Supported +W: http://www.melexis.com +F: include/uapi/linux/mlx7502x.h + MELFAS MIP4 TOUCHSCREEN DRIVER M: Sangwon Jee <[email protected]> S: Supportedinclude/uapi/linux/mlx7502x.h new file +8
-0@@ -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. +* +*/ +Quoted 8 lines
include/uapi/linux/mlx7502x.h line 13
Quoted 5 lines
include/uapi/linux/mlx7502x.h new file +5
-0+#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
Quoted 11 lines
include/uapi/linux/mlx7502x.h line 21
Quoted 8 lines
include/uapi/linux/mlx7502x.h new file +8
-0+/* 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.
Quoted 6 lines
Quoted 5 lines
Quoted 2 lines
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.
Quoted 16 lines
Quoted 5 lines
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
Quoted 8 lines
I hope it helps.
Quoted 7 lines
Quoted 4 lines
Note that the control
V4L2_CID_MLX7502X_TINTis similar toV4L2_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?
Quoted 19 lines
Quoted 19 lines
Quoted 19 lines
Quoted 7 lines
include/uapi/linux/mlx7502x.h line 25
Quoted 4 lines
include/uapi/linux/mlx7502x.h new file +4
-0+/* 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.
Quoted 7 lines
include/uapi/linux/mlx7502x.h line 29
Quoted 4 lines
include/uapi/linux/mlx7502x.h new file +4
-0+/* 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/[email protected]/
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
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/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/
Quoted 8 lines
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
include/uapi/linux/mlx7502x.h new file +2
-0+ +#endif /* __UAPI_MLX7502X_H_ */ Signature · 3 lines
Regards, Laurent Pinchart
Re: [PATCH v2 4/6] media: uapi: Add mlx7502x header file
Quoted 150 lines · Benjamin Mugnier
Hi Volodymyr,
Quoted 5 lines · Volodymyr Kharuk
Quoted 4 lines · Laurent Pinchart
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 :)
Quoted 68 lines
Quoted 68 lines · Volodymyr Kharuk
Quoted 66 lines · Laurent Pinchart
Quoted 42 lines · Volodymyr Kharuk
Quoted 41 lines
Define user controls for mlx7502x driver and update
MAINTAINERSSigned-off-by Volodymyr Kharuk <[email protected]>--- MAINTAINERS | 7 +++++++ include/uapi/linux/mlx7502x.h | 31 +++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100644 include/uapi/linux/mlx7502x.hMAINTAINERS +7
-0@@ -12673,6 +12673,13 @@ S: Supported W: http://www.melexis.com F: drivers/iio/temperature/mlx90632.c +MELEXIS MLX7502X DRIVER +M: Volodymyr Kharuk <[email protected]> +L: [email protected] +S: Supported +W: http://www.melexis.com +F: include/uapi/linux/mlx7502x.h + MELFAS MIP4 TOUCHSCREEN DRIVER M: Sangwon Jee <[email protected]> S: Supportedinclude/uapi/linux/mlx7502x.h new file +8
-0@@ -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. +* +*/ +Quoted 8 lines
include/uapi/linux/mlx7502x.h line 13
Quoted 5 lines
include/uapi/linux/mlx7502x.h new file +5
-0+#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
Quoted 11 lines
include/uapi/linux/mlx7502x.h line 21
Quoted 8 lines
include/uapi/linux/mlx7502x.h new file +8
-0+/* 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.Quoted 6 lines
Quoted 5 lines
Quoted 2 lines
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 changingV4L2_CID_MLX7502X_FMODto 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.Quoted 16 lines
Quoted 5 lines
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/documentQuoted 8 lines
I hope it helps.
Quoted 7 lines
Quoted 4 lines
Note that the control
V4L2_CID_MLX7502X_TINTis similar toV4L2_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 withV4L2_CID_EXPOSUREand 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?Quoted 19 lines
Quoted 19 lines
Quoted 19 lines
Quoted 7 lines
include/uapi/linux/mlx7502x.h line 25
Quoted 4 lines
include/uapi/linux/mlx7502x.h new file +4
-0+/* 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.
Quoted 7 lines
include/uapi/linux/mlx7502x.h line 29
Quoted 4 lines
include/uapi/linux/mlx7502x.h new file +4
-0+/* 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/[email protected]/
Quoted 8 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
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
Quoted 11 lines
[1] https://lore.kernel.org/linux-media/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/Quoted 8 lines
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
include/uapi/linux/mlx7502x.h line 31
Quoted 2 lines
include/uapi/linux/mlx7502x.h new file +2
-0+ +#endif /* __UAPI_MLX7502X_H_ */ --
Regards,Laurent Pinchart
Re: [PATCH v2 4/6] media: uapi: Add mlx7502x header file
In reply to Benjamin MugnierHi Benjamin,
Quoted 89 lines · Benjamin Mugnier
Hi Volodymyr,
Quoted 5 lines · Volodymyr Kharuk
Quoted 4 lines · Laurent Pinchart
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 :)
Quoted 68 lines
Quoted 68 lines · Volodymyr Kharuk
Quoted 66 lines · Laurent Pinchart
Quoted 42 lines · Volodymyr Kharuk
Quoted 41 lines
Define user controls for mlx7502x driver and update
MAINTAINERSSigned-off-by Volodymyr Kharuk <[email protected]>--- MAINTAINERS | 7 +++++++ include/uapi/linux/mlx7502x.h | 31 +++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100644 include/uapi/linux/mlx7502x.hMAINTAINERS +7
-0@@ -12673,6 +12673,13 @@ S: Supported W: http://www.melexis.com F: drivers/iio/temperature/mlx90632.c +MELEXIS MLX7502X DRIVER +M: Volodymyr Kharuk <[email protected]> +L: [email protected] +S: Supported +W: http://www.melexis.com +F: include/uapi/linux/mlx7502x.h + MELFAS MIP4 TOUCHSCREEN DRIVER M: Sangwon Jee <[email protected]> S: Supportedinclude/uapi/linux/mlx7502x.h new file +8
-0@@ -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. +* +*/ +Quoted 8 lines
include/uapi/linux/mlx7502x.h line 13
Quoted 5 lines
include/uapi/linux/mlx7502x.h new file +5
-0+#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
Quoted 11 lines
include/uapi/linux/mlx7502x.h line 21
Quoted 8 lines
include/uapi/linux/mlx7502x.h new file +8
-0+/* 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.Quoted 6 lines
Quoted 5 lines
Quoted 2 lines
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 :)
Quoted 8 lines
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?
Quoted 4 lines
This allows greater ranges.
I suggest changingV4L2_CID_MLX7502X_FMODto 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.
Quoted 23 lines
Quoted 23 lines
Quoted 6 lines
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/documentI hope it helps.
Quoted 5 lines
Quoted 4 lines
Note that the control
V4L2_CID_MLX7502X_TINTis similar toV4L2_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?
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
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 withV4L2_CID_EXPOSUREand 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.
Quoted 40 lines
Quoted 20 lines
Quoted 20 lines
Quoted 20 lines
Quoted 8 lines
include/uapi/linux/mlx7502x.h line 25
Quoted 5 lines
include/uapi/linux/mlx7502x.h new file +4
-0+/* 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.
Quoted 7 lines
include/uapi/linux/mlx7502x.h line 29
Quoted 4 lines
include/uapi/linux/mlx7502x.h new file +4
-0+/* 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/[email protected]/Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
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/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/Quoted 8 lines
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
include/uapi/linux/mlx7502x.h new file +2
-0+ +#endif /* __UAPI_MLX7502X_H_ */ Signature · 3 lines
Regards, Laurent Pinchart
Signature · 3 lines
-- BR, Volodymyr Kharuk
Re: [PATCH v2 5/6] media: dt-bindings: media: i2c: Add mlx7502x camera sensor binding
In reply to Krzysztof KozlowskiHi Krzysztof,
Quoted 55 lines · Krzysztof Kozlowski
Quoted 30 lines · Laurent Pinchart
Quoted 29 lines · Krzysztof Kozlowski
Quoted 27 lines · Laurent Pinchart
Quoted 21 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
One option would be to support the following three compatible values:
compatible = "melexis,mlx75026", "melexis,mlx7502x"; compatible = "melexis,mlx75027", "melexis,mlx7502x"; compatible = "melexis,mlx7502x";The last one only would trigger autodetection. I'm still not sure how to document that properly in bindings though.
Quoted 6 lines
Quoted 3 lines
I missed that part of binding.
Wildcards are not allowed in compatible, so mlx7502x has to go.
Really ? We've had fallback generic compatible strings since the beginning.
Fallback generic compatibles are allowed. Wildcards not. Wildcards were actually never explicitly allowed, they just slipped in to many bindings... We have several discussions on this on mailing list, so no real point to repeat the arguments.
Quoted 4 lines
There is a difference between generic fallback. If the device follows clear specification and version, e.g. "foo-bar-v4", you can use it for generic compatible. This is more common in SoC components. Requirement - there is a clear mapping between versions and SoCs.
I'm not sure to see a clear difference between the two concepts.
Quoted 7 lines
Quoted 3 lines
The clear difference is that you have a versioned and re-usable hardware block plus clear mapping which version goes to which SoC. Version numbers usually start with 1, not with 75025. 75025 is a model name.
How about
Documentation/devicetree/bindings/serial/renesas,scif.yamlfor instance, where the version number isn't known and the SoC name is used instead ? Is that acceptable ?This is the second case I mentioned - family of devices where the family fallback is not allowed to be alone. You cannot use just "renesas,scif" in DTS.
Quoted 7 lines
How should we deal with devices that have different models, where the model is irrelevant to the kernel driver, but relevant to userspace ? Imagine, for instance, a light sensor with 10 models than only differ by the filter they use to tune the sensitivity to different light spectrums ? They are all "compatible" from a software point of view, would the driver need to list all 10 compatible strings ?
I don't understand that example, I mean, what's the problem here? If they are all compatible, you can use only one comaptible, e.g. melexis,mlx75026.
Quoted 7 lines
If you ever need to differentiate it for user-space, you add specific compatible for the model and you have:
melexis,mlx75027, melexis,mlx75026
If user-space needs dedicated compatibles - add them, no one here argues to not to use specific compatibles.
Thanks for explanation. Now I understand the device tree better and the whole idea behind it. I'll remove wildcard and autodetect. Instead I will use of_match_table only.
Quoted 28 lines
Quoted 18 lines
Quoted 13 lines
Quoted 12 lines
For cameras, we often deal with complex pipelines with multiple external devices and multiple IP cores, with drivers that need to communicate with each other to initialize the complete camera system. For instance, each camera-related component in the system registers itself in a media graph that can be queried from userspace and exposes information about all devices, including their model. There's no power up of any device when this query is being performed from userspace. It could possibly be changed (and maybe it should, for reasons unrelated to this discussion), but we're looking at pretty much a complete redesign of V4L2 and MC then.
Quoted 4 lines
Is then autodetection a real use case since you have to power up the sensor each time system boots and this violates privacy? Several I2C sensors do not care about this and they always do it on power up, so aren't we solving here something unimportant?
Quoted 4 lines
In a laptop or tablet with a camera sensor, you likely don't want autodetection. In an industrial device, you don't care, and having the ability to auto-detect the exact sensor model when booting saves cost in the production chain as a single image can work across different models.
We talk about the case here, not generic. Do you want to have autodetection possible here or not?
Best regards,
Krzysztof
Signature · 10 lines
-- Volodymyr Kharuk Embedded Software Engineer Melexis-Ukraine Mykhaila Kotel'nykova St, 4, Kyiv Mobile phone: +38 050 346 5527 www.melexis.com --- The contents of this e-mail are CONFIDENTIAL AND PROPRIETARY. Please read our disclaimer at http://www.melexis.com/mailpolicy
Re: [PATCH v2 4/6] media: uapi: Add mlx7502x header file
In reply to Volodymyr KharukHi Volodymyr,
Quoted 96 lines · Volodymyr Kharuk
Hi Benjamin,
Quoted 93 lines · Benjamin Mugnier
Hi Volodymyr,
Quoted 5 lines · Volodymyr Kharuk
Quoted 4 lines · Laurent Pinchart
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 :)
Quoted 68 lines
Quoted 68 lines · Volodymyr Kharuk
Quoted 66 lines · Laurent Pinchart
Quoted 42 lines · Volodymyr Kharuk
Quoted 41 lines
Define user controls for mlx7502x driver and update
MAINTAINERSSigned-off-by Volodymyr Kharuk <[email protected]>--- MAINTAINERS | 7 +++++++ include/uapi/linux/mlx7502x.h | 31 +++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100644 include/uapi/linux/mlx7502x.hMAINTAINERS +7
-0@@ -12673,6 +12673,13 @@ S: Supported W: http://www.melexis.com F: drivers/iio/temperature/mlx90632.c +MELEXIS MLX7502X DRIVER +M: Volodymyr Kharuk <[email protected]> +L: [email protected] +S: Supported +W: http://www.melexis.com +F: include/uapi/linux/mlx7502x.h + MELFAS MIP4 TOUCHSCREEN DRIVER M: Sangwon Jee <[email protected]> S: Supportedinclude/uapi/linux/mlx7502x.h new file +8
-0@@ -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. +* +*/ +Quoted 8 lines
include/uapi/linux/mlx7502x.h line 13
Quoted 5 lines
include/uapi/linux/mlx7502x.h new file +5
-0+#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
Quoted 11 lines
include/uapi/linux/mlx7502x.h line 21
Quoted 8 lines
include/uapi/linux/mlx7502x.h new file +8
-0+/* 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.Quoted 6 lines
Quoted 5 lines
Quoted 2 lines
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.
Quoted 6 lines
Quoted 4 lines
|-------------------------------------------------------------------------> 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
Quoted 1 line
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.
Quoted 7 lines
Quoted 4 lines
This allows greater ranges.
I suggest changingV4L2_CID_MLX7502X_FMODto 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.
Quoted 23 lines
Quoted 23 lines
Quoted 23 lines
Quoted 6 lines
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/documentI hope it helps.
Quoted 5 lines
Quoted 4 lines
Note that the control
V4L2_CID_MLX7502X_TINTis similar toV4L2_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?
Quoted 6 lines
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
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 withV4L2_CID_EXPOSUREand 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 :)
Quoted 41 lines
Quoted 40 lines
Quoted 20 lines
Quoted 20 lines
Quoted 20 lines
Quoted 8 lines
include/uapi/linux/mlx7502x.h line 25
Quoted 5 lines
include/uapi/linux/mlx7502x.h new file +4
-0+/* 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.
Quoted 7 lines
include/uapi/linux/mlx7502x.h line 29
Quoted 4 lines
include/uapi/linux/mlx7502x.h new file +4
-0+/* 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/[email protected]/Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
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/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/Quoted 8 lines
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
include/uapi/linux/mlx7502x.h new file +2
-0+ +#endif /* __UAPI_MLX7502X_H_ */ Signature · 3 lines
Regards, Laurent Pinchart
Laurent Pinchart
maintainerRe: [PATCH v2 5/6] media: dt-bindings: media: i2c: Add mlx7502x camera sensor binding
In reply to Volodymyr KharukHi Volodymyr,
I've just realized your previous e-mail felt through the cracks. Sorry about that. Please see below for comments.
Quoted 110 lines · Volodymyr Kharuk
Quoted 100 lines · Laurent Pinchart
Quoted 21 lines · Volodymyr Kharuk
Add device tree binding of the mlx7502x and update
MAINTAINERSSigned-off-by Volodymyr Kharuk <[email protected]>--- .../bindings/media/i2c/melexis,mlx7502x.yaml | 146 ++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 147 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yamlDocumentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +6
-0@@ -0,0 +1,146 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/melexis,mlx7502x.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# +Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml line 16
Quoted 10 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +10
-0+title: Melexis ToF 7502x MIPI CSI-2 Sensor + +maintainers: + - Volodymyr Kharuk <[email protected]> + +description: |- + Melexis ToF 7502x sensors has a CSI-2 output. It supports 2 and 4 lanes, + and mipi speeds are 300, 600, 704, 800, 904, 960Mbs. Supported format is RAW12. + Sensor 75026 is QVGA, while 75027 is VGA sensor. + If you use compatible = "melexis,mlx7502x", then autodetect will be called.I'd move this last line as a description of the compatible property, but I'm also not sure this should be mentioned in the DT bindings, as it's a driver implementation detail. I'm actually not sure we should support it with three different compatible values as proposed, as without this documentation users will have a hard time figuring out what compatible value to pick.
One option would be to support the following three compatible values:
compatible = "melexis,mlx75026", "melexis,mlx7502x"; compatible = "melexis,mlx75027", "melexis,mlx7502x"; compatible = "melexis,mlx7502x";The last one only would trigger autodetection. I'm still not sure how to document that properly in bindings though.
Quoted 51 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +51
-0+ +properties: + compatible: + items: + - enum: + - melexis,mlx7502x + - melexis,mlx75026 + - melexis,mlx75027 + + reg: + maxItems: 1 + + assigned-clocks: true + assigned-clock-parents: true + assigned-clock-rates: true + + clocks: + description: Clock frequency 8MHz + maxItems: 1 + + vdda-supply: + description: + Definition of the regulator used as analog power supply(2.7V). + + vddif-supply: + description: + Definition of the regulator used as interface power supply(1.8V). + + vddd-supply: + description: + Definition of the regulator used as digital power supply(1.2V). + + vdmix-supply: + description: + Definition of the regulator used as mixed driver power supply(1.2V). + + reset-gpios: + maxItems: 1 + description: Reset Sensor GPIO Control (active low) + + melexis,trig-gpios: + maxItems: 1 + description: + Hardware Trigger GPIO Control (active low) + When the hardware trigger mode is enabled, this GPIO is used to generate + a low level impulse for about 100us. The impulse triggers a new + measurement cycle. + + melexis,leden-gpios: + maxItems: 1 + description:Quoted 8 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml line 70
Quoted 3 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +3
-0+ Led driver enable GPIO Control (active high) + This GPIO notifies the illumination driver when it is safe to use led + signals from the sensor.As far as I understand this isn't an input of the sensor, but a signal that is driven by the driver and controls the separate illuminator. It shouldn't be specified in this binding, as it's not a property of the sensor. You should instead have a DT node for the illumination driver.
Quoted 5 lines
Yes, you are right. There are illuminators, which are not ready to accept the signal from the sensor, if pin levels are not right. So I need to notify illuminator somehow. Can the illumination driver be as a V4L2 subdevice? Then I can notify the illuminator via
s_stream. In another case it can damage the illuminator. What do you think?
It can be a separate subdev, yes. There are flash-related controls in V4L2, maybe some of them would cover your use cases ? We have a few flash drivers in upstream that you can look at for inspiration, for instance drivers/media/i2c/lm3560.c or drivers/media/i2c/adp1653.c.
Quoted 93 lines
Quoted 28 lines
Quoted 28 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +28
-0+ port: + description: MIPI CSI-2 transmitter port + $ref: /schemas/graph.yaml#/$defs/port-base + + properties: + endpoint: + $ref: /schemas/media/video-interfaces.yaml# + unevaluatedProperties: false + + properties: + data-lanes: + oneOf: + - items: + - const: 1 + - const: 2 + - items: + - const: 1 + - const: 2 + - const: 3 + - const: 4 + + clock-noncontinuous: true + link-frequencies: true + + required: + - data-lanes + - link-frequencies +Quoted 8 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml line 104
Quoted 5 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +5
-0+required: + - compatible + - reg + - clocks + - portLet's make the supplies mandatory too, as the chip can't operate without them.
Ok
Quoted 54 lines
Quoted 54 lines
Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml new file +42
-0+ +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + + i2c { + #address-cells = <1>; + #size-cells = <0>; + + mlx7502x: camera@57 { + compatible = "melexis,mlx7502x"; + reg = <0x57>; + clocks = <&mlx7502x_clk>; + + assigned-clocks = <&mlx7502x_clk>; + assigned-clock-parents = <&mlx7502x_clk_parent>; + assigned-clock-rates = <8000000>; + + vddd-supply = <&mlx_7502x_reg>; + + reset-gpios = <&gpio_exp 6 GPIO_ACTIVE_HIGH>; + melexis,trig-gpios = <&gpio_exp 2 GPIO_ACTIVE_HIGH>; + melexis,leden-gpios = <&gpio_exp 7 GPIO_ACTIVE_HIGH>; + + port { + mlx7502x_out_mipi_csi2: endpoint { + remote-endpoint = <&mipi_csi2_from_mlx7502x>; + data-lanes = <1 2 3 4>; + link-frequencies = /bits/ 64 < 960000000 + 904000000 + 800000000 + 704000000 + 600000000 + 300000000 >; + }; + }; + }; + }; + +...MAINTAINERS +1
-0@@ -12678,6 +12678,7 @@ M: Volodymyr Kharuk <[email protected]> L: [email protected] S: Supported W: http://www.melexis.com +F: Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml F: include/uapi/linux/mlx7502x.h MELFAS MIP4 TOUCHSCREEN DRIVER
Signature · 3 lines
Regards, Laurent Pinchart
Laurent Pinchart
maintainerRe: [PATCH v2 5/6] media: dt-bindings: media: i2c: Add mlx7502x camera sensor binding
In reply to Krzysztof KozlowskiHi Krzysztof,
Very late reply, this had fallen through the cracks.
Quoted 35 lines · Krzysztof Kozlowski
Quoted 34 lines · Laurent Pinchart
Quoted 32 lines · Krzysztof Kozlowski
Quoted 21 lines · Laurent Pinchart
Quoted 21 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
One option would be to support the following three compatible values:
compatible = "melexis,mlx75026", "melexis,mlx7502x"; compatible = "melexis,mlx75027", "melexis,mlx7502x"; compatible = "melexis,mlx7502x";The last one only would trigger autodetection. I'm still not sure how to document that properly in bindings though.
Quoted 6 lines
Quoted 3 lines
I missed that part of binding.
Wildcards are not allowed in compatible, so mlx7502x has to go.
Really ? We've had fallback generic compatible strings since the beginning.
Fallback generic compatibles are allowed. Wildcards not. Wildcards were actually never explicitly allowed, they just slipped in to many bindings... We have several discussions on this on mailing list, so no real point to repeat the arguments.
Quoted 6 lines
Quoted 4 lines
There is a difference between generic fallback. If the device follows clear specification and version, e.g. "foo-bar-v4", you can use it for generic compatible. This is more common in SoC components. Requirement - there is a clear mapping between versions and SoCs.
I'm not sure to see a clear difference between the two concepts.
The clear difference is that you have a versioned and re-usable hardware block plus clear mapping which version goes to which SoC. Version numbers usually start with 1, not with 75025. 75025 is a model name.
Quoted 7 lines
Quoted 3 lines
How about
Documentation/devicetree/bindings/serial/renesas,scif.yamlfor instance, where the version number isn't known and the SoC name is used instead ? Is that acceptable ?This is the second case I mentioned - family of devices where the family fallback is not allowed to be alone. You cannot use just "renesas,scif" in DTS.
OK. Does this mean you are fine with
compatible = "melexis,mlx75026", "melexis,mlx7502x"; compatible = "melexis,mlx75027", "melexis,mlx7502x";
where "melexis,mlx7502x" is considered to be the family fallback, but not
compatible = "melexis,mlx7502x";
alone ?
Quoted 11 lines
Quoted 6 lines
How should we deal with devices that have different models, where the model is irrelevant to the kernel driver, but relevant to userspace ? Imagine, for instance, a light sensor with 10 models than only differ by the filter they use to tune the sensitivity to different light spectrums ? They are all "compatible" from a software point of view, would the driver need to list all 10 compatible strings ?
I don't understand that example, I mean, what's the problem here? If they are all compatible, you can use only one comaptible, e.g. melexis,mlx75026.
Quoted 7 lines
If you ever need to differentiate it for user-space, you add specific compatible for the model and you have:
melexis,mlx75027, melexis,mlx75026
If user-space needs dedicated compatibles - add them, no one here argues to not to use specific compatibles.
OK.
Quoted 16 lines
Quoted 16 lines
Quoted 15 lines
Quoted 10 lines
For cameras, we often deal with complex pipelines with multiple external devices and multiple IP cores, with drivers that need to communicate with each other to initialize the complete camera system. For instance, each camera-related component in the system registers itself in a media graph that can be queried from userspace and exposes information about all devices, including their model. There's no power up of any device when this query is being performed from userspace. It could possibly be changed (and maybe it should, for reasons unrelated to this discussion), but we're looking at pretty much a complete redesign of V4L2 and MC then.
Is then autodetection a real use case since you have to power up the sensor each time system boots and this violates privacy? Several I2C sensors do not care about this and they always do it on power up, so aren't we solving here something unimportant?
Quoted 7 lines
Quoted 4 lines
In a laptop or tablet with a camera sensor, you likely don't want autodetection. In an industrial device, you don't care, and having the ability to auto-detect the exact sensor model when booting saves cost in the production chain as a single image can work across different models.
We talk about the case here, not generic. Do you want to have autodetection possible here or not?
I'd like to support auto-detection, but not make it mandatory. Assuming a family of chips supported by one driver with hardware that makes auto-detection possible, I have use cases where I specifically don't want auto-detection as it would have undesirable side effects at probe time, and other use cases where I want auto-detection as it lowers the costs in the production chain. I thus need to be able to specify, in DT, whether to use auto-detection or not, and when not using auto-detection, specify the exact chip model.
Signature · 3 lines
Regards, Laurent Pinchart
Re: [PATCH v2 5/6] media: dt-bindings: media: i2c: Add mlx7502x camera sensor binding
Quoted 53 lines · Laurent Pinchart
Hi Krzysztof,
Very late reply, this had fallen through the cracks.
Quoted 35 lines · Krzysztof Kozlowski
Quoted 34 lines · Laurent Pinchart
Quoted 32 lines · Krzysztof Kozlowski
Quoted 21 lines · Laurent Pinchart
Quoted 21 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
One option would be to support the following three compatible values:
compatible = "melexis,mlx75026", "melexis,mlx7502x"; compatible = "melexis,mlx75027", "melexis,mlx7502x"; compatible = "melexis,mlx7502x";The last one only would trigger autodetection. I'm still not sure how to document that properly in bindings though.
Quoted 6 lines
Quoted 3 lines
I missed that part of binding.
Wildcards are not allowed in compatible, so mlx7502x has to go.
Really ? We've had fallback generic compatible strings since the beginning.
Fallback generic compatibles are allowed. Wildcards not. Wildcards were actually never explicitly allowed, they just slipped in to many bindings... We have several discussions on this on mailing list, so no real point to repeat the arguments.
Quoted 6 lines
Quoted 4 lines
There is a difference between generic fallback. If the device follows clear specification and version, e.g. "foo-bar-v4", you can use it for generic compatible. This is more common in SoC components. Requirement - there is a clear mapping between versions and SoCs.
I'm not sure to see a clear difference between the two concepts.
The clear difference is that you have a versioned and re-usable hardware block plus clear mapping which version goes to which SoC. Version numbers usually start with 1, not with 75025. 75025 is a model name.
Quoted 7 lines
Quoted 3 lines
How about
Documentation/devicetree/bindings/serial/renesas,scif.yamlfor instance, where the version number isn't known and the SoC name is used instead ? Is that acceptable ?This is the second case I mentioned - family of devices where the family fallback is not allowed to be alone. You cannot use just "renesas,scif" in DTS.
OK. Does this mean you are fine with
compatible = "melexis,mlx75026", "melexis,mlx7502x"; compatible = "melexis,mlx75027", "melexis,mlx7502x";
Quoted 6 lines
where "melexis,mlx7502x" is considered to be the family fallback, but not
compatible = "melexis,mlx7502x";alone ?
Correct.
(...)
Quoted 9 lines
Quoted 8 lines
Quoted 5 lines
In a laptop or tablet with a camera sensor, you likely don't want autodetection. In an industrial device, you don't care, and having the ability to auto-detect the exact sensor model when booting saves cost in the production chain as a single image can work across different models.
We talk about the case here, not generic. Do you want to have autodetection possible here or not?
Quoted 8 lines
I'd like to support auto-detection, but not make it mandatory. Assuming a family of chips supported by one driver with hardware that makes auto-detection possible, I have use cases where I specifically don't want auto-detection as it would have undesirable side effects at probe time, and other use cases where I want auto-detection as it lowers the costs in the production chain. I thus need to be able to specify, in DT, whether to use auto-detection or not, and when not using auto-detection, specify the exact chip model.
OK, I understand. This however stretches the Devicetree approach - you are putting OS policy of device probing into the DT binding. What's more, it serves only Linux' purpose. If other OS/software is fine with auto-detection on first use (thus no privacy concerns), then all this discussion is not relevant. The binding is independent of OS, thus we should not align it to our specific OS behavior.
Maybe Linux needs some generic runtime knob to turn on/off autodetection for all devices. It does not look like a job for DT.
Best regards,
Krzysztof
Laurent Pinchart
maintainerRe: [PATCH v2 5/6] media: dt-bindings: media: i2c: Add mlx7502x camera sensor binding
Hi Krzysztof,
Quoted 86 lines · Krzysztof Kozlowski
Quoted 49 lines · Laurent Pinchart
Quoted 35 lines · Krzysztof Kozlowski
Quoted 34 lines · Laurent Pinchart
Quoted 32 lines · Krzysztof Kozlowski
Quoted 21 lines · Laurent Pinchart
Quoted 21 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
One option would be to support the following three compatible values:
compatible = "melexis,mlx75026", "melexis,mlx7502x"; compatible = "melexis,mlx75027", "melexis,mlx7502x"; compatible = "melexis,mlx7502x";The last one only would trigger autodetection. I'm still not sure how to document that properly in bindings though.
Quoted 6 lines
Quoted 3 lines
I missed that part of binding.
Wildcards are not allowed in compatible, so mlx7502x has to go.
Really ? We've had fallback generic compatible strings since the beginning.
Fallback generic compatibles are allowed. Wildcards not. Wildcards were actually never explicitly allowed, they just slipped in to many bindings... We have several discussions on this on mailing list, so no real point to repeat the arguments.
Quoted 6 lines
Quoted 4 lines
There is a difference between generic fallback. If the device follows clear specification and version, e.g. "foo-bar-v4", you can use it for generic compatible. This is more common in SoC components. Requirement - there is a clear mapping between versions and SoCs.
I'm not sure to see a clear difference between the two concepts.
The clear difference is that you have a versioned and re-usable hardware block plus clear mapping which version goes to which SoC. Version numbers usually start with 1, not with 75025. 75025 is a model name.
Quoted 7 lines
Quoted 3 lines
How about
Documentation/devicetree/bindings/serial/renesas,scif.yamlfor instance, where the version number isn't known and the SoC name is used instead ? Is that acceptable ?This is the second case I mentioned - family of devices where the family fallback is not allowed to be alone. You cannot use just "renesas,scif" in DTS.
OK. Does this mean you are fine with
compatible = "melexis,mlx75026", "melexis,mlx7502x"; compatible = "melexis,mlx75027", "melexis,mlx7502x";Quoted 6 lines
where "melexis,mlx7502x" is considered to be the family fallback, but not
compatible = "melexis,mlx7502x";alone ?
Correct.
(...)
Quoted 9 lines
Quoted 8 lines
Quoted 5 lines
In a laptop or tablet with a camera sensor, you likely don't want autodetection. In an industrial device, you don't care, and having the ability to auto-detect the exact sensor model when booting saves cost in the production chain as a single image can work across different models.
We talk about the case here, not generic. Do you want to have autodetection possible here or not?
Quoted 8 lines
I'd like to support auto-detection, but not make it mandatory. Assuming a family of chips supported by one driver with hardware that makes auto-detection possible, I have use cases where I specifically don't want auto-detection as it would have undesirable side effects at probe time, and other use cases where I want auto-detection as it lowers the costs in the production chain. I thus need to be able to specify, in DT, whether to use auto-detection or not, and when not using auto-detection, specify the exact chip model.
OK, I understand. This however stretches the Devicetree approach - you are putting OS policy of device probing into the DT binding. What's more, it serves only Linux' purpose. If other OS/software is fine with auto-detection on first use (thus no privacy concerns), then all this discussion is not relevant. The binding is independent of OS, thus we should not align it to our specific OS behavior.
Quoted 2 lines
Maybe Linux needs some generic runtime knob to turn on/off autodetection for all devices. It does not look like a job for DT.
If we want to be able to run without auto-detection, regardless of the operating system, we need to specify the exact model in DT, otherwise a driver wouldn't be able to identify the device.
If we want to use auto-detection, that's for the purpose of simplifying system integration, with a single DT that covers multiple device variants.
Those two use cases thus require DT binding that allow for both options, specifying an exact model, or being more generic. I agree that deciding to auto-detect based on what compatible strings are specified may be specific to a particular Linux driver, but the fact that we need the two options to support both use cases isn't OS-specific in my opinion.
Signature · 3 lines
Regards, Laurent Pinchart