[1/2] media: dt-bindings: media: i2c: Add ST VGXY61 camera sensor binding

  Add device tree binding for the ST VGXY61 camera sensor, and update
MAINTAINERS file.

Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
---
 .../bindings/media/i2c/st,st-vgxy61.yaml      | 134 ++++++++++++++++++
 MAINTAINERS                                   |  10 ++
 2 files changed, 144 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml
  

Hi Benjamin

cc Laurent and Sakari as maintainers of video-interfaces.yaml

On Thu, 10 Mar 2022 at 13:37, Benjamin Mugnier
<benjamin.mugnier@foss.st.com> wrote:
>
> Add device tree binding for the ST VGXY61 camera sensor, and update
> MAINTAINERS file.
>
> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> ---
>  .../bindings/media/i2c/st,st-vgxy61.yaml      | 134 ++++++++++++++++++
>  MAINTAINERS                                   |  10 ++
>  2 files changed, 144 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml b/Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml
> new file mode 100644
> index 000000000000..8740ed2623e4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml
> @@ -0,0 +1,134 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright (c) 2022 STMicroelectronics SA.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/st,st-vgxy61.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: STMicroelectronics VGxy61 HDR Global Shutter Sensor Family Device Tree Bindings
> +
> +maintainers:
> +  - Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> +  - Sylvain Petinot <sylvain.petinot@foss.st.com>
> +
> +description: |-
> +  STMicroelectronics VGxy61 family has a CSI-2 output port. CSI-2 output is a
> +  quad lanes 800Mbps per lane.
> +  Supported formats are RAW8, RAW10, RAW12, RAW14 and RAW16.
> +  Following part number are supported
> +  - VG5661 and VG6661 are 1.6 Mpx (1464 x 1104) monochrome and color sensors.
> +  Maximum frame rate is 75 fps.
> +  - VG5761 and VG6761 are 2.3 Mpx (1944 x 1204) monochrome and color sensors.
> +  Maximum frame rate is 60 fps.
> +
> +properties:
> +  compatible:
> +    const: st,st-vgxy61
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    description:
> +      Input clock for the sensor.
> +    items:
> +      - const: xclk
> +
> +  VCORE-supply:
> +    description:
> +      Sensor digital core supply. Must be 1.2 volts.
> +
> +  VDDIO-supply:
> +    description:
> +      Sensor digital IO supply. Must be 1.8 volts.
> +
> +  VANA-supply:
> +    description:
> +      Sensor analog supply. Must be 2.8 volts.
> +
> +  reset-gpios:
> +    description:
> +      Reference to the GPIO connected to the reset pin, if any.
> +      This is an active low signal to the vgxy61.
> +
> +  invert-gpios-polarity:
> +    description:
> +      If gpios polarity should be inversed

s/inversed/inverted

> +    type: boolean
> +
> +  slave-mode:
> +    description:
> +      If the sensor operates in slave mode
> +    type: boolean

This is one I've been meaning to raise for a while.
Is DT the correct place to be configuring hardware sync options for
image sensors? (There may be the linguistic discussions over master /
slave terminology too).
We also have IMX477 and a number of other sensors that support
external sync control of some form.

As I see it, there are nominally 3 settings - disabled (reduces EMC
noise), generate syncs, and receive syncs.
For test purposes it would be useful to be able to switch between
generate and receive modes at runtime, so that would make it a control
instead of being fixed in DT.

If it should be configured in DT, then how does ACPI need to handle it?

If DT is the correct place to define the role, should it be in
video-interfaces.yaml as an optional property, instead of being a
sensor specific binding?

Sorry, more questions rather than answers.

  Dave

> +    #TODO check all this or copy from elsewhere
> +  port:
> +    $ref: /schemas/graph.yaml#/$defs/port-base
> +    additionalProperties: false
> +
> +    properties:
> +      endpoint:
> +        $ref: /schemas/media/video-interfaces.yaml#
> +        unevaluatedProperties: false
> +
> +        properties:
> +          clock-lane:
> +            description:
> +              Clock lane index
> +            maxItems: 1
> +
> +          data-lanes:
> +            description:
> +              CSI lanes to use
> +            items:
> +              - const: 1
> +              - const: 2
> +              - const: 3
> +              - const: 4
> +
> +          remote-endpoint: true
> +
> +        required:
> +          - clock-lane
> +          - data-lanes
> +          - remote-endpoint
> +
> +required:
> +  - compatible
> +  - clocks
> +  - clock-names
> +  - VCORE-supply
> +  - VDDIO-supply
> +  - VANA-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        vgxy61: csi2tx@10 {
> +            compatible = "st,st-vgxy61";
> +            reg = <0x10>;
> +            status = "okay";
> +            clocks = <&clk_ext_camera>;
> +            clock-names = "xclk";
> +            VCORE-supply = <&v1v2>;
> +            VDDIO-supply = <&v1v8>;
> +            VANA-supply = <&v2v8>;
> +            reset-gpios = <&mfxgpio 18 GPIO_ACTIVE_LOW>;
> +            port {
> +                ep0: endpoint {
> +                    clock-lane = <0>;
> +                    data-lanes = <1 2 3 4>;
> +                    remote-endpoint = <&mipi_csi2_out>;
> +                };
> +            };
> +        };
> +    };
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 83d27b57016f..f358d15f68a0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18297,6 +18297,16 @@ S:     Maintained
>  F:     Documentation/hwmon/stpddc60.rst
>  F:     drivers/hwmon/pmbus/stpddc60.c
>
> +ST VGXY61 DRIVER
> +M:     Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> +M:     Sylvain Petinot <sylvain.petinot@foss.st.com>
> +L:     linux-media@vger.kernel.org
> +S:     Maintained
> +T:     git git://linuxtv.org/media_tree.git
> +F:     Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.txt
> +F:     drivers/media/i2c/st-vgxy61.c
> +
> +
>  ST VL53L0X ToF RANGER(I2C) IIO DRIVER
>  M:     Song Qiang <songqiang1304521@gmail.com>
>  L:     linux-iio@vger.kernel.org
> --
> 2.25.1
>
  

Hi Dave,

Thank you for your review.

On 10/03/2022 16:38, Dave Stevenson wrote:
> Hi Benjamin
> 
> cc Laurent and Sakari as maintainers of video-interfaces.yaml
> 
> On Thu, 10 Mar 2022 at 13:37, Benjamin Mugnier
> <benjamin.mugnier@foss.st.com> wrote:
>>
>> Add device tree binding for the ST VGXY61 camera sensor, and update
>> MAINTAINERS file.
>>
>> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
>> ---
>>  .../bindings/media/i2c/st,st-vgxy61.yaml      | 134 ++++++++++++++++++
>>  MAINTAINERS                                   |  10 ++
>>  2 files changed, 144 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml b/Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml
>> new file mode 100644
>> index 000000000000..8740ed2623e4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml
>> @@ -0,0 +1,134 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +# Copyright (c) 2022 STMicroelectronics SA.
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/media/i2c/st,st-vgxy61.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: STMicroelectronics VGxy61 HDR Global Shutter Sensor Family Device Tree Bindings
>> +
>> +maintainers:
>> +  - Benjamin Mugnier <benjamin.mugnier@foss.st.com>
>> +  - Sylvain Petinot <sylvain.petinot@foss.st.com>
>> +
>> +description: |-
>> +  STMicroelectronics VGxy61 family has a CSI-2 output port. CSI-2 output is a
>> +  quad lanes 800Mbps per lane.
>> +  Supported formats are RAW8, RAW10, RAW12, RAW14 and RAW16.
>> +  Following part number are supported
>> +  - VG5661 and VG6661 are 1.6 Mpx (1464 x 1104) monochrome and color sensors.
>> +  Maximum frame rate is 75 fps.
>> +  - VG5761 and VG6761 are 2.3 Mpx (1944 x 1204) monochrome and color sensors.
>> +  Maximum frame rate is 60 fps.
>> +
>> +properties:
>> +  compatible:
>> +    const: st,st-vgxy61
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  clock-names:
>> +    description:
>> +      Input clock for the sensor.
>> +    items:
>> +      - const: xclk
>> +
>> +  VCORE-supply:
>> +    description:
>> +      Sensor digital core supply. Must be 1.2 volts.
>> +
>> +  VDDIO-supply:
>> +    description:
>> +      Sensor digital IO supply. Must be 1.8 volts.
>> +
>> +  VANA-supply:
>> +    description:
>> +      Sensor analog supply. Must be 2.8 volts.
>> +
>> +  reset-gpios:
>> +    description:
>> +      Reference to the GPIO connected to the reset pin, if any.
>> +      This is an active low signal to the vgxy61.
>> +
>> +  invert-gpios-polarity:
>> +    description:
>> +      If gpios polarity should be inversed
> 
> s/inversed/inverted
> 

Ok.

>> +    type: boolean
>> +
>> +  slave-mode:
>> +    description:
>> +      If the sensor operates in slave mode
>> +    type: boolean
> 
> This is one I've been meaning to raise for a while.
> Is DT the correct place to be configuring hardware sync options for
> image sensors? (There may be the linguistic discussions over master /
> slave terminology too).
> We also have IMX477 and a number of other sensors that support
> external sync control of some form.
> 
> As I see it, there are nominally 3 settings - disabled (reduces EMC
> noise), generate syncs, and receive syncs.
> For test purposes it would be useful to be able to switch between
> generate and receive modes at runtime, so that would make it a control
> instead of being fixed in DT.
> 
> If it should be configured in DT, then how does ACPI need to handle it?
> 
> If DT is the correct place to define the role, should it be in
> video-interfaces.yaml as an optional property, instead of being a
> sensor specific binding?
> 
> Sorry, more questions rather than answers.
> 
>   Dave

Maybe I can provide additional info on this sensor to help find an answer.
The "slave mode" has 2 settings: enabled or disabled. If disabled you are in master mode ('generate sync' and 'disabled' modes Dave mentionned, they are the same here), and if enabled you are in slave mode ('receive sync').
As you said he master sends frame sync signals to the slave each frame acquired, this allows both sensors to synchronize themselves.

I put this in the device tree as we only use it for 3D stereocam boards which already have 2 sensors on them, meaning this is hardware specific. I don't have any use case where we manually wire 2 sensors on 2 separate boards.
One good point you mentioned is that I may not always want run this board in master/slaver, and both sensors could run on master mode without interacting with each other, thus justifying a dedicated v4l2 control.

Any ideas on how to name it instead of "slave mode" for coherency between sensors?


Regard,

Benjamin

> 
>> +    #TODO check all this or copy from elsewhere

Just noticed this and will remove it.

>> +  port:
>> +    $ref: /schemas/graph.yaml#/$defs/port-base
>> +    additionalProperties: false
>> +
>> +    properties:
>> +      endpoint:
>> +        $ref: /schemas/media/video-interfaces.yaml#
>> +        unevaluatedProperties: false
>> +
>> +        properties:
>> +          clock-lane:
>> +            description:
>> +              Clock lane index
>> +            maxItems: 1
>> +
>> +          data-lanes:
>> +            description:
>> +              CSI lanes to use
>> +            items:
>> +              - const: 1
>> +              - const: 2
>> +              - const: 3
>> +              - const: 4
>> +
>> +          remote-endpoint: true
>> +
>> +        required:
>> +          - clock-lane
>> +          - data-lanes
>> +          - remote-endpoint
>> +
>> +required:
>> +  - compatible
>> +  - clocks
>> +  - clock-names
>> +  - VCORE-supply
>> +  - VDDIO-supply
>> +  - VANA-supply
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/gpio/gpio.h>
>> +    i2c {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +        vgxy61: csi2tx@10 {
>> +            compatible = "st,st-vgxy61";
>> +            reg = <0x10>;
>> +            status = "okay";
>> +            clocks = <&clk_ext_camera>;
>> +            clock-names = "xclk";
>> +            VCORE-supply = <&v1v2>;
>> +            VDDIO-supply = <&v1v8>;
>> +            VANA-supply = <&v2v8>;
>> +            reset-gpios = <&mfxgpio 18 GPIO_ACTIVE_LOW>;
>> +            port {
>> +                ep0: endpoint {
>> +                    clock-lane = <0>;
>> +                    data-lanes = <1 2 3 4>;
>> +                    remote-endpoint = <&mipi_csi2_out>;
>> +                };
>> +            };
>> +        };
>> +    };
>> +...
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 83d27b57016f..f358d15f68a0 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -18297,6 +18297,16 @@ S:     Maintained
>>  F:     Documentation/hwmon/stpddc60.rst
>>  F:     drivers/hwmon/pmbus/stpddc60.c
>>
>> +ST VGXY61 DRIVER
>> +M:     Benjamin Mugnier <benjamin.mugnier@foss.st.com>
>> +M:     Sylvain Petinot <sylvain.petinot@foss.st.com>
>> +L:     linux-media@vger.kernel.org
>> +S:     Maintained
>> +T:     git git://linuxtv.org/media_tree.git
>> +F:     Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.txt
>> +F:     drivers/media/i2c/st-vgxy61.c
>> +
>> +
>>  ST VL53L0X ToF RANGER(I2C) IIO DRIVER
>>  M:     Song Qiang <songqiang1304521@gmail.com>
>>  L:     linux-iio@vger.kernel.org
>> --
>> 2.25.1
>>
  

Hi Benjamin,

On Fri, Mar 11, 2022 at 12:25:38PM +0100, Benjamin Mugnier wrote:
> Hi Dave,
> 
> Thank you for your review.
> 
> On 10/03/2022 16:38, Dave Stevenson wrote:
> > Hi Benjamin
> > 
> > cc Laurent and Sakari as maintainers of video-interfaces.yaml
> > 
> > On Thu, 10 Mar 2022 at 13:37, Benjamin Mugnier
> > <benjamin.mugnier@foss.st.com> wrote:
> >>
> >> Add device tree binding for the ST VGXY61 camera sensor, and update
> >> MAINTAINERS file.
> >>
> >> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> >> ---
> >>  .../bindings/media/i2c/st,st-vgxy61.yaml      | 134 ++++++++++++++++++
> >>  MAINTAINERS                                   |  10 ++
> >>  2 files changed, 144 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml b/Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml
> >> new file mode 100644
> >> index 000000000000..8740ed2623e4
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml
> >> @@ -0,0 +1,134 @@
> >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >> +# Copyright (c) 2022 STMicroelectronics SA.
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/media/i2c/st,st-vgxy61.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: STMicroelectronics VGxy61 HDR Global Shutter Sensor Family Device Tree Bindings
> >> +
> >> +maintainers:
> >> +  - Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> >> +  - Sylvain Petinot <sylvain.petinot@foss.st.com>
> >> +
> >> +description: |-
> >> +  STMicroelectronics VGxy61 family has a CSI-2 output port. CSI-2 output is a
> >> +  quad lanes 800Mbps per lane.
> >> +  Supported formats are RAW8, RAW10, RAW12, RAW14 and RAW16.
> >> +  Following part number are supported
> >> +  - VG5661 and VG6661 are 1.6 Mpx (1464 x 1104) monochrome and color sensors.
> >> +  Maximum frame rate is 75 fps.
> >> +  - VG5761 and VG6761 are 2.3 Mpx (1944 x 1204) monochrome and color sensors.
> >> +  Maximum frame rate is 60 fps.
> >> +
> >> +properties:
> >> +  compatible:
> >> +    const: st,st-vgxy61
> >> +
> >> +  reg:
> >> +    maxItems: 1
> >> +
> >> +  clocks:
> >> +    maxItems: 1
> >> +
> >> +  clock-names:
> >> +    description:
> >> +      Input clock for the sensor.
> >> +    items:
> >> +      - const: xclk

Do you need this if you have a single clock?

Also see Documentation/driver-api/media/camera-sensor.rst .

> >> +
> >> +  VCORE-supply:
> >> +    description:
> >> +      Sensor digital core supply. Must be 1.2 volts.
> >> +
> >> +  VDDIO-supply:
> >> +    description:
> >> +      Sensor digital IO supply. Must be 1.8 volts.
> >> +
> >> +  VANA-supply:
> >> +    description:
> >> +      Sensor analog supply. Must be 2.8 volts.
> >> +
> >> +  reset-gpios:
> >> +    description:
> >> +      Reference to the GPIO connected to the reset pin, if any.
> >> +      This is an active low signal to the vgxy61.
> >> +
> >> +  invert-gpios-polarity:
> >> +    description:
> >> +      If gpios polarity should be inversed
> > 
> > s/inversed/inverted
> > 
> 
> Ok.
> 
> >> +    type: boolean
> >> +
> >> +  slave-mode:
> >> +    description:
> >> +      If the sensor operates in slave mode
> >> +    type: boolean
> > 
> > This is one I've been meaning to raise for a while.
> > Is DT the correct place to be configuring hardware sync options for
> > image sensors? (There may be the linguistic discussions over master /
> > slave terminology too).
> > We also have IMX477 and a number of other sensors that support
> > external sync control of some form.
> > 
> > As I see it, there are nominally 3 settings - disabled (reduces EMC
> > noise), generate syncs, and receive syncs.
> > For test purposes it would be useful to be able to switch between
> > generate and receive modes at runtime, so that would make it a control
> > instead of being fixed in DT.
> > 
> > If it should be configured in DT, then how does ACPI need to handle it?
> > 
> > If DT is the correct place to define the role, should it be in
> > video-interfaces.yaml as an optional property, instead of being a
> > sensor specific binding?
> > 
> > Sorry, more questions rather than answers.
> > 
> >   Dave
> 
> Maybe I can provide additional info on this sensor to help find an
> answer. The "slave mode" has 2 settings: enabled or disabled. If disabled
> you are in master mode ('generate sync' and 'disabled' modes Dave
> mentionned, they are the same here), and if enabled you are in slave mode
> ('receive sync'). As you said he master sends frame sync signals to the
> slave each frame acquired, this allows both sensors to synchronize
> themselves.
> 
> I put this in the device tree as we only use it for 3D stereocam boards
> which already have 2 sensors on them, meaning this is hardware specific.
> I don't have any use case where we manually wire 2 sensors on 2 separate
> boards. One good point you mentioned is that I may not always want run
> this board in master/slaver, and both sensors could run on master mode
> without interacting with each other, thus justifying a dedicated v4l2
> control.
> 
> Any ideas on how to name it instead of "slave mode" for coherency between
> sensors?

How is this wired? The slave-mode property documentation explicitly refers
to synchronisation signals that do not exists in CSI-2.

> 
> 
> Regard,
> 
> Benjamin
> 
> > 
> >> +    #TODO check all this or copy from elsewhere
> 
> Just noticed this and will remove it.
> 
> >> +  port:
> >> +    $ref: /schemas/graph.yaml#/$defs/port-base
> >> +    additionalProperties: false
> >> +
> >> +    properties:
> >> +      endpoint:
> >> +        $ref: /schemas/media/video-interfaces.yaml#
> >> +        unevaluatedProperties: false
> >> +
> >> +        properties:
> >> +          clock-lane:
> >> +            description:
> >> +              Clock lane index
> >> +            maxItems: 1

Does the device support lane reordering? If not, please drop.

> >> +
> >> +          data-lanes:
> >> +            description:
> >> +              CSI lanes to use
> >> +            items:
> >> +              - const: 1
> >> +              - const: 2
> >> +              - const: 3
> >> +              - const: 4

Which lane configurations does the device support? If it's four lanes only,
then you can drop this property, too.

> >> +
> >> +          remote-endpoint: true
> >> +
> >> +        required:
> >> +          - clock-lane
> >> +          - data-lanes
> >> +          - remote-endpoint

Listing remote-endpoint here isn't needed as this comes from the schema.

> >> +
> >> +required:
> >> +  - compatible
> >> +  - clocks
> >> +  - clock-names
> >> +  - VCORE-supply
> >> +  - VDDIO-supply
> >> +  - VANA-supply
> >> +
> >> +additionalProperties: false
> >> +
> >> +examples:
> >> +  - |
> >> +    #include <dt-bindings/gpio/gpio.h>
> >> +    i2c {
> >> +        #address-cells = <1>;
> >> +        #size-cells = <0>;
> >> +        vgxy61: csi2tx@10 {
> >> +            compatible = "st,st-vgxy61";
> >> +            reg = <0x10>;
> >> +            status = "okay";
> >> +            clocks = <&clk_ext_camera>;
> >> +            clock-names = "xclk";
> >> +            VCORE-supply = <&v1v2>;
> >> +            VDDIO-supply = <&v1v8>;
> >> +            VANA-supply = <&v2v8>;
> >> +            reset-gpios = <&mfxgpio 18 GPIO_ACTIVE_LOW>;
> >> +            port {
> >> +                ep0: endpoint {
> >> +                    clock-lane = <0>;
> >> +                    data-lanes = <1 2 3 4>;
> >> +                    remote-endpoint = <&mipi_csi2_out>;
> >> +                };
> >> +            };
> >> +        };
> >> +    };
> >> +...
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index 83d27b57016f..f358d15f68a0 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -18297,6 +18297,16 @@ S:     Maintained
> >>  F:     Documentation/hwmon/stpddc60.rst
> >>  F:     drivers/hwmon/pmbus/stpddc60.c
> >>
> >> +ST VGXY61 DRIVER
> >> +M:     Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> >> +M:     Sylvain Petinot <sylvain.petinot@foss.st.com>
> >> +L:     linux-media@vger.kernel.org
> >> +S:     Maintained
> >> +T:     git git://linuxtv.org/media_tree.git
> >> +F:     Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.txt
> >> +F:     drivers/media/i2c/st-vgxy61.c
> >> +

Extra newline.

> >> +
> >>  ST VL53L0X ToF RANGER(I2C) IIO DRIVER
> >>  M:     Song Qiang <songqiang1304521@gmail.com>
> >>  L:     linux-iio@vger.kernel.org
> >> --
> >> 2.25.1
> >>
  

Hello,

On Fri, Mar 11, 2022 at 03:13:11PM +0200, Sakari Ailus wrote:
> On Fri, Mar 11, 2022 at 12:25:38PM +0100, Benjamin Mugnier wrote:
> > On 10/03/2022 16:38, Dave Stevenson wrote:
> > > On Thu, 10 Mar 2022 at 13:37, Benjamin Mugnier wrote:
> > >>
> > >> Add device tree binding for the ST VGXY61 camera sensor, and update
> > >> MAINTAINERS file.
> > >>
> > >> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> > >> ---
> > >>  .../bindings/media/i2c/st,st-vgxy61.yaml      | 134 ++++++++++++++++++
> > >>  MAINTAINERS                                   |  10 ++
> > >>  2 files changed, 144 insertions(+)
> > >>  create mode 100644 Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml
> > >>
> > >> diff --git a/Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml b/Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml
> > >> new file mode 100644
> > >> index 000000000000..8740ed2623e4
> > >> --- /dev/null
> > >> +++ b/Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml
> > >> @@ -0,0 +1,134 @@
> > >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > >> +# Copyright (c) 2022 STMicroelectronics SA.
> > >> +%YAML 1.2
> > >> +---
> > >> +$id: http://devicetree.org/schemas/media/i2c/st,st-vgxy61.yaml#
> > >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > >> +
> > >> +title: STMicroelectronics VGxy61 HDR Global Shutter Sensor Family Device Tree Bindings
> > >> +
> > >> +maintainers:
> > >> +  - Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> > >> +  - Sylvain Petinot <sylvain.petinot@foss.st.com>
> > >> +
> > >> +description: |-
> > >> +  STMicroelectronics VGxy61 family has a CSI-2 output port. CSI-2 output is a
> > >> +  quad lanes 800Mbps per lane.
> > >> +  Supported formats are RAW8, RAW10, RAW12, RAW14 and RAW16.
> > >> +  Following part number are supported
> > >> +  - VG5661 and VG6661 are 1.6 Mpx (1464 x 1104) monochrome and color sensors.
> > >> +  Maximum frame rate is 75 fps.
> > >> +  - VG5761 and VG6761 are 2.3 Mpx (1944 x 1204) monochrome and color sensors.
> > >> +  Maximum frame rate is 60 fps.
> > >> +
> > >> +properties:
> > >> +  compatible:
> > >> +    const: st,st-vgxy61
> > >> +
> > >> +  reg:
> > >> +    maxItems: 1
> > >> +
> > >> +  clocks:
> > >> +    maxItems: 1
> > >> +
> > >> +  clock-names:
> > >> +    description:
> > >> +      Input clock for the sensor.
> > >> +    items:
> > >> +      - const: xclk
> 
> Do you need this if you have a single clock?
> 
> Also see Documentation/driver-api/media/camera-sensor.rst .
> 
> > >> +
> > >> +  VCORE-supply:
> > >> +    description:
> > >> +      Sensor digital core supply. Must be 1.2 volts.
> > >> +
> > >> +  VDDIO-supply:
> > >> +    description:
> > >> +      Sensor digital IO supply. Must be 1.8 volts.
> > >> +
> > >> +  VANA-supply:
> > >> +    description:
> > >> +      Sensor analog supply. Must be 2.8 volts.
> > >> +
> > >> +  reset-gpios:
> > >> +    description:
> > >> +      Reference to the GPIO connected to the reset pin, if any.
> > >> +      This is an active low signal to the vgxy61.
> > >> +
> > >> +  invert-gpios-polarity:
> > >> +    description:
> > >> +      If gpios polarity should be inversed
> > > 
> > > s/inversed/inverted
> > > 
> > 
> > Ok.
> > 
> > >> +    type: boolean
> > >> +
> > >> +  slave-mode:
> > >> +    description:
> > >> +      If the sensor operates in slave mode
> > >> +    type: boolean
> > > 
> > > This is one I've been meaning to raise for a while.
> > > Is DT the correct place to be configuring hardware sync options for
> > > image sensors? (There may be the linguistic discussions over master /
> > > slave terminology too).
> > > We also have IMX477 and a number of other sensors that support
> > > external sync control of some form.
> > > 
> > > As I see it, there are nominally 3 settings - disabled (reduces EMC
> > > noise), generate syncs, and receive syncs.
> > > For test purposes it would be useful to be able to switch between
> > > generate and receive modes at runtime, so that would make it a control
> > > instead of being fixed in DT.
> > > 
> > > If it should be configured in DT, then how does ACPI need to handle it?
> > > 
> > > If DT is the correct place to define the role, should it be in
> > > video-interfaces.yaml as an optional property, instead of being a
> > > sensor specific binding?
> > > 
> > > Sorry, more questions rather than answers.
> > > 
> > >   Dave
> > 
> > Maybe I can provide additional info on this sensor to help find an
> > answer. The "slave mode" has 2 settings: enabled or disabled. If disabled
> > you are in master mode ('generate sync' and 'disabled' modes Dave
> > mentionned, they are the same here), and if enabled you are in slave mode
> > ('receive sync'). As you said he master sends frame sync signals to the
> > slave each frame acquired, this allows both sensors to synchronize
> > themselves.
> > 
> > I put this in the device tree as we only use it for 3D stereocam boards
> > which already have 2 sensors on them, meaning this is hardware specific.
> > I don't have any use case where we manually wire 2 sensors on 2 separate
> > boards. One good point you mentioned is that I may not always want run
> > this board in master/slaver, and both sensors could run on master mode
> > without interacting with each other, thus justifying a dedicated v4l2
> > control.
> > 
> > Any ideas on how to name it instead of "slave mode" for coherency between
> > sensors?
> 
> How is this wired? The slave-mode property documentation explicitly refers
> to synchronisation signals that do not exists in CSI-2.

What is slave mode in this case ? Does it only mean that the sensor is
externally triggered, or is there something else ? For parallel
interfaces with H/V sync there's a possibility of the H/V sync signals
being inputs instead of outputs, but that's not applicable to CSI-2.

> > >> +    #TODO check all this or copy from elsewhere
> > 
> > Just noticed this and will remove it.
> > 
> > >> +  port:
> > >> +    $ref: /schemas/graph.yaml#/$defs/port-base
> > >> +    additionalProperties: false
> > >> +
> > >> +    properties:
> > >> +      endpoint:
> > >> +        $ref: /schemas/media/video-interfaces.yaml#
> > >> +        unevaluatedProperties: false
> > >> +
> > >> +        properties:
> > >> +          clock-lane:
> > >> +            description:
> > >> +              Clock lane index
> > >> +            maxItems: 1
> 
> Does the device support lane reordering? If not, please drop.
> 
> > >> +
> > >> +          data-lanes:
> > >> +            description:
> > >> +              CSI lanes to use
> > >> +            items:
> > >> +              - const: 1
> > >> +              - const: 2
> > >> +              - const: 3
> > >> +              - const: 4
> 
> Which lane configurations does the device support? If it's four lanes only,
> then you can drop this property, too.
> 
> > >> +
> > >> +          remote-endpoint: true
> > >> +
> > >> +        required:
> > >> +          - clock-lane
> > >> +          - data-lanes
> > >> +          - remote-endpoint
> 
> Listing remote-endpoint here isn't needed as this comes from the schema.
> 
> > >> +
> > >> +required:
> > >> +  - compatible
> > >> +  - clocks
> > >> +  - clock-names
> > >> +  - VCORE-supply
> > >> +  - VDDIO-supply
> > >> +  - VANA-supply
> > >> +
> > >> +additionalProperties: false
> > >> +
> > >> +examples:
> > >> +  - |
> > >> +    #include <dt-bindings/gpio/gpio.h>
> > >> +    i2c {
> > >> +        #address-cells = <1>;
> > >> +        #size-cells = <0>;
> > >> +        vgxy61: csi2tx@10 {
> > >> +            compatible = "st,st-vgxy61";
> > >> +            reg = <0x10>;
> > >> +            status = "okay";
> > >> +            clocks = <&clk_ext_camera>;
> > >> +            clock-names = "xclk";
> > >> +            VCORE-supply = <&v1v2>;
> > >> +            VDDIO-supply = <&v1v8>;
> > >> +            VANA-supply = <&v2v8>;
> > >> +            reset-gpios = <&mfxgpio 18 GPIO_ACTIVE_LOW>;
> > >> +            port {
> > >> +                ep0: endpoint {
> > >> +                    clock-lane = <0>;
> > >> +                    data-lanes = <1 2 3 4>;
> > >> +                    remote-endpoint = <&mipi_csi2_out>;
> > >> +                };
> > >> +            };
> > >> +        };
> > >> +    };
> > >> +...
> > >> diff --git a/MAINTAINERS b/MAINTAINERS
> > >> index 83d27b57016f..f358d15f68a0 100644
> > >> --- a/MAINTAINERS
> > >> +++ b/MAINTAINERS
> > >> @@ -18297,6 +18297,16 @@ S:     Maintained
> > >>  F:     Documentation/hwmon/stpddc60.rst
> > >>  F:     drivers/hwmon/pmbus/stpddc60.c
> > >>
> > >> +ST VGXY61 DRIVER
> > >> +M:     Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> > >> +M:     Sylvain Petinot <sylvain.petinot@foss.st.com>
> > >> +L:     linux-media@vger.kernel.org
> > >> +S:     Maintained
> > >> +T:     git git://linuxtv.org/media_tree.git
> > >> +F:     Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.txt
> > >> +F:     drivers/media/i2c/st-vgxy61.c
> > >> +
> 
> Extra newline.
> 
> > >> +
> > >>  ST VL53L0X ToF RANGER(I2C) IIO DRIVER
> > >>  M:     Song Qiang <songqiang1304521@gmail.com>
> > >>  L:     linux-iio@vger.kernel.org
  

Hi Sakari and Laurent,

Thank you for your reviews.

On 13/03/2022 15:53, Laurent Pinchart wrote:
> Hello,
> 
> On Fri, Mar 11, 2022 at 03:13:11PM +0200, Sakari Ailus wrote:
>> On Fri, Mar 11, 2022 at 12:25:38PM +0100, Benjamin Mugnier wrote:
>>> On 10/03/2022 16:38, Dave Stevenson wrote:
>>>> On Thu, 10 Mar 2022 at 13:37, Benjamin Mugnier wrote:
>>>>>
>>>>> Add device tree binding for the ST VGXY61 camera sensor, and update
>>>>> MAINTAINERS file.
>>>>>
>>>>> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
>>>>> ---
>>>>>  .../bindings/media/i2c/st,st-vgxy61.yaml      | 134 ++++++++++++++++++
>>>>>  MAINTAINERS                                   |  10 ++
>>>>>  2 files changed, 144 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml b/Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..8740ed2623e4
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml
>>>>> @@ -0,0 +1,134 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>>> +# Copyright (c) 2022 STMicroelectronics SA.
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/media/i2c/st,st-vgxy61.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: STMicroelectronics VGxy61 HDR Global Shutter Sensor Family Device Tree Bindings
>>>>> +
>>>>> +maintainers:
>>>>> +  - Benjamin Mugnier <benjamin.mugnier@foss.st.com>
>>>>> +  - Sylvain Petinot <sylvain.petinot@foss.st.com>
>>>>> +
>>>>> +description: |-
>>>>> +  STMicroelectronics VGxy61 family has a CSI-2 output port. CSI-2 output is a
>>>>> +  quad lanes 800Mbps per lane.
>>>>> +  Supported formats are RAW8, RAW10, RAW12, RAW14 and RAW16.
>>>>> +  Following part number are supported
>>>>> +  - VG5661 and VG6661 are 1.6 Mpx (1464 x 1104) monochrome and color sensors.
>>>>> +  Maximum frame rate is 75 fps.
>>>>> +  - VG5761 and VG6761 are 2.3 Mpx (1944 x 1204) monochrome and color sensors.
>>>>> +  Maximum frame rate is 60 fps.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: st,st-vgxy61
>>>>> +
>>>>> +  reg:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  clocks:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  clock-names:
>>>>> +    description:
>>>>> +      Input clock for the sensor.
>>>>> +    items:
>>>>> +      - const: xclk
>>
>> Do you need this if you have a single clock?
>>
>> Also see Documentation/driver-api/media/camera-sensor.rst .
>>

Yes the sensor only uses 1 clock.
I will move this to assigned-clocks and use clk_get_rate instead.

>>>>> +
>>>>> +  VCORE-supply:
>>>>> +    description:
>>>>> +      Sensor digital core supply. Must be 1.2 volts.
>>>>> +
>>>>> +  VDDIO-supply:
>>>>> +    description:
>>>>> +      Sensor digital IO supply. Must be 1.8 volts.
>>>>> +
>>>>> +  VANA-supply:
>>>>> +    description:
>>>>> +      Sensor analog supply. Must be 2.8 volts.
>>>>> +
>>>>> +  reset-gpios:
>>>>> +    description:
>>>>> +      Reference to the GPIO connected to the reset pin, if any.
>>>>> +      This is an active low signal to the vgxy61.
>>>>> +
>>>>> +  invert-gpios-polarity:
>>>>> +    description:
>>>>> +      If gpios polarity should be inversed
>>>>
>>>> s/inversed/inverted
>>>>
>>>
>>> Ok.
>>>
>>>>> +    type: boolean
>>>>> +
>>>>> +  slave-mode:
>>>>> +    description:
>>>>> +      If the sensor operates in slave mode
>>>>> +    type: boolean
>>>>
>>>> This is one I've been meaning to raise for a while.
>>>> Is DT the correct place to be configuring hardware sync options for
>>>> image sensors? (There may be the linguistic discussions over master /
>>>> slave terminology too).
>>>> We also have IMX477 and a number of other sensors that support
>>>> external sync control of some form.
>>>>
>>>> As I see it, there are nominally 3 settings - disabled (reduces EMC
>>>> noise), generate syncs, and receive syncs.
>>>> For test purposes it would be useful to be able to switch between
>>>> generate and receive modes at runtime, so that would make it a control
>>>> instead of being fixed in DT.
>>>>
>>>> If it should be configured in DT, then how does ACPI need to handle it?
>>>>
>>>> If DT is the correct place to define the role, should it be in
>>>> video-interfaces.yaml as an optional property, instead of being a
>>>> sensor specific binding?
>>>>
>>>> Sorry, more questions rather than answers.
>>>>
>>>>   Dave
>>>
>>> Maybe I can provide additional info on this sensor to help find an
>>> answer. The "slave mode" has 2 settings: enabled or disabled. If disabled
>>> you are in master mode ('generate sync' and 'disabled' modes Dave
>>> mentionned, they are the same here), and if enabled you are in slave mode
>>> ('receive sync'). As you said he master sends frame sync signals to the
>>> slave each frame acquired, this allows both sensors to synchronize
>>> themselves.
>>>
>>> I put this in the device tree as we only use it for 3D stereocam boards
>>> which already have 2 sensors on them, meaning this is hardware specific.
>>> I don't have any use case where we manually wire 2 sensors on 2 separate
>>> boards. One good point you mentioned is that I may not always want run
>>> this board in master/slaver, and both sensors could run on master mode
>>> without interacting with each other, thus justifying a dedicated v4l2
>>> control.
>>>
>>> Any ideas on how to name it instead of "slave mode" for coherency between
>>> sensors?
>>
>> How is this wired? The slave-mode property documentation explicitly refers
>> to synchronisation signals that do not exists in CSI-2.

Each sensor have a FRAMESTART output pin and an EXTSYNC input pin.
In master mode, the sensor will trigger a rising pulse for each acquired frame on its FRAMESTART pin.
In slave mode, the sensor will wait for a rising pulse on its EXTSYNC pin to trigger a frame acquisition.
What is typically done for sterocams:

┌───────┐FRAMESTART     EXTSYNC┌───────┐
│sensor1├─────────────────────>│sensor2│
└───────┘                      └───────┘

Sensor 1 has its FRAMESTART pin wired to sensor 2's EXTSYNC pin. From this you can set the sensor 1 to master mode, this will guarantee that on each acquired frame, it will emit a rising pulse on its FRAMESTART pin, wired to the sensor 2 EXTSYNC pin. If the sensor 2 is set to slave mode, this will triggering a frame acquisition for sensor 2.
You can also set both sensors to master mode and they will acquire frames independently.

> 
> What is slave mode in this case ? Does it only mean that the sensor is
> externally triggered, or is there something else ? For parallel
> interfaces with H/V sync there's a possibility of the H/V sync signals
> being inputs instead of outputs, but that's not applicable to CSI-2.
> 

Exactly, if the sensor is running in slave mode the frame acquisition is triggered for each rising pulse on its EXTSYNC pin, supposedly by another sensor.
I'm not sure I got what you meant for parallel interfaces, but afaik this behavior seems close to the vertical sync you are mentioning.

Tell me if you need more info.

>>>>> +    #TODO check all this or copy from elsewhere
>>>
>>> Just noticed this and will remove it.
>>>
>>>>> +  port:
>>>>> +    $ref: /schemas/graph.yaml#/$defs/port-base
>>>>> +    additionalProperties: false
>>>>> +
>>>>> +    properties:
>>>>> +      endpoint:
>>>>> +        $ref: /schemas/media/video-interfaces.yaml#
>>>>> +        unevaluatedProperties: false
>>>>> +
>>>>> +        properties:
>>>>> +          clock-lane:
>>>>> +            description:
>>>>> +              Clock lane index
>>>>> +            maxItems: 1
>>
>> Does the device support lane reordering? If not, please drop.
>>

The sensor does not support lane reordering, hence the clock is always on lane 0. Thank you I will remove this.

>>>>> +
>>>>> +          data-lanes:
>>>>> +            description:
>>>>> +              CSI lanes to use
>>>>> +            items:
>>>>> +              - const: 1
>>>>> +              - const: 2
>>>>> +              - const: 3
>>>>> +              - const: 4
>>
>> Which lane configurations does the device support? If it's four lanes only,
>> then you can drop this property, too.
>>

It supports 1, 2 or 4 lanes, and you can choose which lane you want to use. This property is the array of lane indexes you want to use.

>>>>> +
>>>>> +          remote-endpoint: true
>>>>> +
>>>>> +        required:
>>>>> +          - clock-lane
>>>>> +          - data-lanes
>>>>> +          - remote-endpoint
>>
>> Listing remote-endpoint here isn't needed as this comes from the schema.
>>

Ok.

>>>>> +
>>>>> +required:
>>>>> +  - compatible
>>>>> +  - clocks
>>>>> +  - clock-names
>>>>> +  - VCORE-supply
>>>>> +  - VDDIO-supply
>>>>> +  - VANA-supply
>>>>> +
>>>>> +additionalProperties: false
>>>>> +
>>>>> +examples:
>>>>> +  - |
>>>>> +    #include <dt-bindings/gpio/gpio.h>
>>>>> +    i2c {
>>>>> +        #address-cells = <1>;
>>>>> +        #size-cells = <0>;
>>>>> +        vgxy61: csi2tx@10 {
>>>>> +            compatible = "st,st-vgxy61";
>>>>> +            reg = <0x10>;
>>>>> +            status = "okay";
>>>>> +            clocks = <&clk_ext_camera>;
>>>>> +            clock-names = "xclk";
>>>>> +            VCORE-supply = <&v1v2>;
>>>>> +            VDDIO-supply = <&v1v8>;
>>>>> +            VANA-supply = <&v2v8>;
>>>>> +            reset-gpios = <&mfxgpio 18 GPIO_ACTIVE_LOW>;
>>>>> +            port {
>>>>> +                ep0: endpoint {
>>>>> +                    clock-lane = <0>;
>>>>> +                    data-lanes = <1 2 3 4>;
>>>>> +                    remote-endpoint = <&mipi_csi2_out>;
>>>>> +                };
>>>>> +            };
>>>>> +        };
>>>>> +    };
>>>>> +...
>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>> index 83d27b57016f..f358d15f68a0 100644
>>>>> --- a/MAINTAINERS
>>>>> +++ b/MAINTAINERS
>>>>> @@ -18297,6 +18297,16 @@ S:     Maintained
>>>>>  F:     Documentation/hwmon/stpddc60.rst
>>>>>  F:     drivers/hwmon/pmbus/stpddc60.c
>>>>>
>>>>> +ST VGXY61 DRIVER
>>>>> +M:     Benjamin Mugnier <benjamin.mugnier@foss.st.com>
>>>>> +M:     Sylvain Petinot <sylvain.petinot@foss.st.com>
>>>>> +L:     linux-media@vger.kernel.org
>>>>> +S:     Maintained
>>>>> +T:     git git://linuxtv.org/media_tree.git
>>>>> +F:     Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.txt
>>>>> +F:     drivers/media/i2c/st-vgxy61.c
>>>>> +
>>
>> Extra newline.
>>

Ok.

>>>>> +
>>>>>  ST VL53L0X ToF RANGER(I2C) IIO DRIVER
>>>>>  M:     Song Qiang <songqiang1304521@gmail.com>
>>>>>  L:     linux-iio@vger.kernel.org
>