diff mbox series

[v2,5/6] media: dt-bindings: media: i2c: Add mlx7502x camera sensor binding

Message ID 712c1acff963238e685cbd5c4a1b91f0ec7f9061.1657786765.git.vkh@melexis.com
State New
Headers show
Series media: i2c: mlx7502x ToF camera support | expand

Commit Message

Volodymyr Kharuk July 14, 2022, 8:34 a.m. UTC
Add device tree binding of the mlx7502x and update MAINTAINERS

Signed-off-by: Volodymyr Kharuk <vkh@melexis.com>
---
 .../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

Comments

Krzysztof Kozlowski July 14, 2022, 8:41 a.m. UTC | #1
On 14/07/2022 10:34, Volodymyr Kharuk wrote:
> Add device tree binding of the mlx7502x and update MAINTAINERS
> 
> Signed-off-by: Volodymyr Kharuk <vkh@melexis.com>

Wrong subject prefix. Remove trailing "binding".

> ---
>  .../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
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml b/Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml
> new file mode 100644
> index 000000000000..4ac91f7a26b6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml
> @@ -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 <vkh@melexis.com>
> +
> +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.

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

> +
> +  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)

> +
> +  vddif-supply:
> +    description:
> +      Definition of the regulator used as interface power supply(1.8V).

s/Definition of the regulator used as //

> +
> +  vddd-supply:
> +    description:
> +      Definition of the regulator used as digital power supply(1.2V).

s/Definition of the regulator used as //

> +
> +  vdmix-supply:
> +    description:
> +      Definition of the regulator used as mixed driver power supply(1.2V).

s/Definition of the regulator used as //

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

You do not need these two, they come from video-interfaces.yaml

> +
> +        required:
> +          - data-lanes
> +          - link-frequencies
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - port
> +



Best regards,
Krzysztof
Laurent Pinchart July 14, 2022, 10:06 a.m. UTC | #2
Hi Volodymyr,

Thank you for the patch.

On Thu, Jul 14, 2022 at 11:34:47AM +0300, Volodymyr Kharuk wrote:
> Add device tree binding of the mlx7502x and update MAINTAINERS
> 
> Signed-off-by: Volodymyr Kharuk <vkh@melexis.com>
> ---
>  .../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
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml b/Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml
> new file mode 100644
> index 000000000000..4ac91f7a26b6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml
> @@ -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 <vkh@melexis.com>
> +
> +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.

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

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.

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

Let's make the supplies mandatory too, as the chip can't operate without
them.

> +
> +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 >;
> +                };
> +            };
> +        };
> +    };
> +
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1a68d888ee14..b00a726bb3db 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12678,6 +12678,7 @@ M:	Volodymyr Kharuk <vkh@melexis.com>
>  L:	linux-media@vger.kernel.org
>  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
Krzysztof Kozlowski July 14, 2022, 10:35 a.m. UTC | #3
On 14/07/2022 12:06, Laurent Pinchart wrote:
> Hi Volodymyr,
> 
> Thank you for the patch.
> 
> On Thu, Jul 14, 2022 at 11:34:47AM +0300, Volodymyr Kharuk wrote:
>> Add device tree binding of the mlx7502x and update MAINTAINERS
>>
>> Signed-off-by: Volodymyr Kharuk <vkh@melexis.com>
>> ---
>>  .../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
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml b/Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml
>> new file mode 100644
>> index 000000000000..4ac91f7a26b6
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml
>> @@ -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 <vkh@melexis.com>
>> +
>> +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.

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
Laurent Pinchart July 14, 2022, 10:45 a.m. UTC | #4
On Thu, Jul 14, 2022 at 12:35:52PM +0200, Krzysztof Kozlowski wrote:
> On 14/07/2022 12:06, Laurent Pinchart wrote:
> > Hi Volodymyr,
> > 
> > Thank you for the patch.
> > 
> > On Thu, Jul 14, 2022 at 11:34:47AM +0300, Volodymyr Kharuk wrote:
> >> Add device tree binding of the mlx7502x and update MAINTAINERS
> >>
> >> Signed-off-by: Volodymyr Kharuk <vkh@melexis.com>
> >> ---
> >>  .../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
> >>
> >> diff --git a/Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml b/Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml
> >> new file mode 100644
> >> index 000000000000..4ac91f7a26b6
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml
> >> @@ -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 <vkh@melexis.com>
> >> +
> >> +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.
> 
> 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.

> 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.
Laurent Pinchart July 14, 2022, 11:12 a.m. UTC | #5
On Thu, Jul 14, 2022 at 01:00:24PM +0200, Krzysztof Kozlowski wrote:
> On 14/07/2022 12:45, Laurent Pinchart wrote:
> > On Thu, Jul 14, 2022 at 12:35:52PM +0200, Krzysztof Kozlowski wrote:
> >> On 14/07/2022 12:06, Laurent Pinchart wrote:
> >>> Hi Volodymyr,
> >>>
> >>> Thank you for the patch.
> >>>
> >>> On Thu, Jul 14, 2022 at 11:34:47AM +0300, Volodymyr Kharuk wrote:
> >>>> Add device tree binding of the mlx7502x and update MAINTAINERS
> >>>>
> >>>> Signed-off-by: Volodymyr Kharuk <vkh@melexis.com>
> >>>> ---
> >>>>  .../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
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml b/Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml
> >>>> new file mode 100644
> >>>> index 000000000000..4ac91f7a26b6
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml
> >>>> @@ -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 <vkh@melexis.com>
> >>>> +
> >>>> +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.
> >>
> >> 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.

I'm not sure to see a clear difference between the two concepts.

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

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

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.
Laurent Pinchart July 14, 2022, 11:29 a.m. UTC | #6
On Thu, Jul 14, 2022 at 01:23:41PM +0200, Krzysztof Kozlowski wrote:
> On 14/07/2022 13:12, Laurent Pinchart wrote:
> >>>>> 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.
> >>>
> >>> 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.
> > 
> > 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 ?

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

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.
Volodymyr Kharuk July 15, 2022, 3:32 p.m. UTC | #7
Hi Laurent,

Thank you for your review,

On Thu, Jul 14, 2022 at 01:06:35PM +0300, Laurent Pinchart wrote:
> Hi Volodymyr,
> 
> Thank you for the patch.
> 
> On Thu, Jul 14, 2022 at 11:34:47AM +0300, Volodymyr Kharuk wrote:
> > Add device tree binding of the mlx7502x and update MAINTAINERS
> > 
> > Signed-off-by: Volodymyr Kharuk <vkh@melexis.com>
> > ---
> >  .../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
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml b/Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml
> > new file mode 100644
> > index 000000000000..4ac91f7a26b6
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml
> > @@ -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 <vkh@melexis.com>
> > +
> > +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.
> 
> > +
> > +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.
> 
> 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?
> 
> > +  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
> 
> Let's make the supplies mandatory too, as the chip can't operate without
> them.
Ok
> 
> > +
> > +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 >;
> > +                };
> > +            };
> > +        };
> > +    };
> > +
> > +...
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 1a68d888ee14..b00a726bb3db 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12678,6 +12678,7 @@ M:	Volodymyr Kharuk <vkh@melexis.com>
> >  L:	linux-media@vger.kernel.org
> >  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
> 
> -- 
> Regards,
> 
> Laurent Pinchart
Volodymyr Kharuk July 20, 2022, 2:54 p.m. UTC | #8
Hi Krzysztof,

On Thu, Jul 14, 2022 at 01:56:13PM +0200, Krzysztof Kozlowski wrote:
> On 14/07/2022 13:29, Laurent Pinchart wrote:
> > On Thu, Jul 14, 2022 at 01:23:41PM +0200, Krzysztof Kozlowski wrote:
> >> On 14/07/2022 13:12, Laurent Pinchart wrote:
> >>>>>>> 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.
> >>>>>
> >>>>> 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.
> >>>
> >>> 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 ?
> 
> 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.
> 
> > 
> > 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.
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.
> 
> 
> >>> 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?
> > 
> > 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
Laurent Pinchart Feb. 6, 2023, 10:45 a.m. UTC | #9
Hi Krzysztof,

Very late reply, this had fallen through the cracks.

On Thu, Jul 14, 2022 at 01:56:13PM +0200, Krzysztof Kozlowski wrote:
> On 14/07/2022 13:29, Laurent Pinchart wrote:
> > On Thu, Jul 14, 2022 at 01:23:41PM +0200, Krzysztof Kozlowski wrote:
> >> On 14/07/2022 13:12, Laurent Pinchart wrote:
> >>>>>>> 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.
> >>>>>
> >>>>> 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.
> >>>
> >>> 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 ?
> 
> 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 ?

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

OK.

> >>> 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?
> > 
> > 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.
Krzysztof Kozlowski Feb. 6, 2023, 6:20 p.m. UTC | #10
On 06/02/2023 11:45, Laurent Pinchart wrote:
> Hi Krzysztof,
> 
> Very late reply, this had fallen through the cracks.
> 
> On Thu, Jul 14, 2022 at 01:56:13PM +0200, Krzysztof Kozlowski wrote:
>> On 14/07/2022 13:29, Laurent Pinchart wrote:
>>> On Thu, Jul 14, 2022 at 01:23:41PM +0200, Krzysztof Kozlowski wrote:
>>>> On 14/07/2022 13:12, Laurent Pinchart wrote:
>>>>>>>>> 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.
>>>>>>>
>>>>>>> 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.
>>>>>
>>>>> 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 ?
>>
>> 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 ?

Correct.

(...)

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

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
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml b/Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml
new file mode 100644
index 000000000000..4ac91f7a26b6
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml
@@ -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 <vkh@melexis.com>
+
+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 >;
+                };
+            };
+        };
+    };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 1a68d888ee14..b00a726bb3db 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12678,6 +12678,7 @@  M:	Volodymyr Kharuk <vkh@melexis.com>
 L:	linux-media@vger.kernel.org
 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