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@xxxxxxxxxxx> wrote: > >> > >> Add device tree binding for the ST VGXY61 camera sensor, and update > >> MAINTAINERS file. > >> > >> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@xxxxxxxxxxx> > >> --- > >> .../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@xxxxxxxxxxx> > >> + - Sylvain Petinot <sylvain.petinot@xxxxxxxxxxx> > >> + > >> +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@xxxxxxxxxxx> > >> +M: Sylvain Petinot <sylvain.petinot@xxxxxxxxxxx> > >> +L: linux-media@xxxxxxxxxxxxxxx > >> +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@xxxxxxxxx> > >> L: linux-iio@xxxxxxxxxxxxxxx > >> -- > >> 2.25.1 > >> -- Kind regards, Sakari Ailus
