| Message ID | 20221208104006.316606-5-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | i2c-atr and FPDLink | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Thu, Dec 08, 2022 at 12:40:02PM +0200, Tomi Valkeinen wrote: > Add DT bindings for TI DS90UB953 FPDLink-3 Serializer. Seems like this and DS90UB913 binding could be combined. I couldn't spot a difference. In the subjects, drop 'binding for'. The prefix says this is a binding. Maybe add 'Serializer'. > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > .../bindings/media/i2c/ti,ds90ub953.yaml | 112 ++++++++++++++++++ > 1 file changed, 112 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub953.yaml > > diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953.yaml > new file mode 100644 > index 000000000000..fd7d25d93e2c > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953.yaml > @@ -0,0 +1,112 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/i2c/ti,ds90ub953.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Texas Instruments DS90UB953 FPD-Link 3 Serializer > + > +maintainers: > + - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > + > +description: > + The TI DS90UB953 is an FPD-Link 3 video serializer for MIPI CSI-2. > + > +properties: > + compatible: > + enum: > + - ti,ds90ub953-q1 > + - ti,ds90ub971-q1 > + > + '#gpio-cells': > + const: 2 > + > + gpio-controller: true > + > + '#clock-cells': > + const: 0 > + > + ports: > + $ref: /schemas/graph.yaml#/properties/ports > + > + properties: > + port@0: > + $ref: /schemas/graph.yaml#/$defs/port-base > + unevaluatedProperties: false > + description: CSI-2 input port > + > + properties: > + endpoint: > + $ref: /schemas/media/video-interfaces.yaml# > + unevaluatedProperties: false > + > + port@1: > + $ref: /schemas/graph.yaml#/properties/port > + unevaluatedProperties: false > + description: FPD-Link 3 output port > + > + i2c: > + $ref: /schemas/i2c/i2c-controller.yaml# > + unevaluatedProperties: false > + > +required: > + - compatible > + - '#gpio-cells' > + - gpio-controller > + - '#clock-cells' > + - ports > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + > + serializer { > + compatible = "ti,ds90ub953-q1"; > + > + gpio-controller; > + #gpio-cells = <2>; > + > + #clock-cells = <0>; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + ub953_in: endpoint { > + clock-lanes = <0>; > + data-lanes = <1 2 3 4>; > + remote-endpoint = <&sensor_out>; > + }; > + }; > + > + port@1 { > + reg = <1>; > + endpoint { > + remote-endpoint = <&deser_fpd_in>; > + }; > + }; > + }; > + > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + sensor@1a { > + compatible = "sony,imx274"; > + reg = <0x1a>; > + > + reset-gpios = <&serializer 0 GPIO_ACTIVE_LOW>; > + > + port { > + sensor_out: endpoint { > + remote-endpoint = <&ub953_in>; > + }; > + }; > + }; > + }; > + }; > +... > -- > 2.34.1 > >
Hi Tomi, Thank you for the patch. On Thu, Dec 08, 2022 at 12:40:02PM +0200, Tomi Valkeinen wrote: > Add DT bindings for TI DS90UB953 FPDLink-3 Serializer. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > .../bindings/media/i2c/ti,ds90ub953.yaml | 112 ++++++++++++++++++ > 1 file changed, 112 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub953.yaml > > diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953.yaml > new file mode 100644 > index 000000000000..fd7d25d93e2c > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953.yaml > @@ -0,0 +1,112 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/i2c/ti,ds90ub953.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Texas Instruments DS90UB953 FPD-Link 3 Serializer > + > +maintainers: > + - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > + > +description: > + The TI DS90UB953 is an FPD-Link 3 video serializer for MIPI CSI-2. > + > +properties: > + compatible: > + enum: > + - ti,ds90ub953-q1 > + - ti,ds90ub971-q1 > + > + '#gpio-cells': > + const: 2 I would add a description here, to tell what the cells correspond to. In particular, the first cell selects the GPIO_* pin number, it would be nice to document that its value should be in the range [0, 3]. Same comment for patch 3/8 (DS90UB913 bindings). There you could also mention that GPO2 and the output clock are mutually exclusive. > + > + gpio-controller: true > + No need for clocks and clock-names for the reference input clock ? Or is this because you support sync mode only for now ? > + '#clock-cells': > + const: 0 > + > + ports: > + $ref: /schemas/graph.yaml#/properties/ports > + > + properties: > + port@0: > + $ref: /schemas/graph.yaml#/$defs/port-base > + unevaluatedProperties: false > + description: CSI-2 input port > + > + properties: > + endpoint: > + $ref: /schemas/media/video-interfaces.yaml# > + unevaluatedProperties: false Should the data-lanes property be required for the CSI-2 input ? > + > + port@1: > + $ref: /schemas/graph.yaml#/properties/port > + unevaluatedProperties: false > + description: FPD-Link 3 output port > + > + i2c: > + $ref: /schemas/i2c/i2c-controller.yaml# > + unevaluatedProperties: false > + > +required: > + - compatible > + - '#gpio-cells' > + - gpio-controller > + - '#clock-cells' > + - ports > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + > + serializer { > + compatible = "ti,ds90ub953-q1"; > + > + gpio-controller; > + #gpio-cells = <2>; > + > + #clock-cells = <0>; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + ub953_in: endpoint { > + clock-lanes = <0>; > + data-lanes = <1 2 3 4>; > + remote-endpoint = <&sensor_out>; > + }; > + }; > + > + port@1 { > + reg = <1>; > + endpoint { > + remote-endpoint = <&deser_fpd_in>; > + }; > + }; > + }; > + > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + sensor@1a { > + compatible = "sony,imx274"; > + reg = <0x1a>; > + > + reset-gpios = <&serializer 0 GPIO_ACTIVE_LOW>; Maybe add clocks = <&serializer>; clock-names = "inck"; to showcase the clock connection ? > + > + port { > + sensor_out: endpoint { > + remote-endpoint = <&ub953_in>; > + }; > + }; > + }; > + }; > + }; > +...
On 11/12/2022 19:34, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Thu, Dec 08, 2022 at 12:40:02PM +0200, Tomi Valkeinen wrote: >> Add DT bindings for TI DS90UB953 FPDLink-3 Serializer. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> .../bindings/media/i2c/ti,ds90ub953.yaml | 112 ++++++++++++++++++ >> 1 file changed, 112 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub953.yaml >> >> diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953.yaml >> new file mode 100644 >> index 000000000000..fd7d25d93e2c >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953.yaml >> @@ -0,0 +1,112 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/media/i2c/ti,ds90ub953.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Texas Instruments DS90UB953 FPD-Link 3 Serializer >> + >> +maintainers: >> + - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> + >> +description: >> + The TI DS90UB953 is an FPD-Link 3 video serializer for MIPI CSI-2. >> + >> +properties: >> + compatible: >> + enum: >> + - ti,ds90ub953-q1 >> + - ti,ds90ub971-q1 >> + >> + '#gpio-cells': >> + const: 2 > > I would add a description here, to tell what the cells correspond to. In > particular, the first cell selects the GPIO_* pin number, it would be > nice to document that its value should be in the range [0, 3]. > > Same comment for patch 3/8 (DS90UB913 bindings). There you could also > mention that GPO2 and the output clock are mutually exclusive. Yep. I have added this for ub913: First cell is the GPO pin number, second cell is the flags. The GPO pin number must be in range of [0, 3]. Note that GPOs 2 and 3 are not available in external oscillator mode. and this for ub953: First cell is the GPIO pin number, second cell is the flags. The GPIO pin number must be in range of [0, 3]. >> + >> + gpio-controller: true >> + > > No need for clocks and clock-names for the reference input clock ? Or is > this because you support sync mode only for now ? Right, I don't have the clock on my hw, but it's probably better to add it to the binding already. >> + '#clock-cells': >> + const: 0 >> + >> + ports: >> + $ref: /schemas/graph.yaml#/properties/ports >> + >> + properties: >> + port@0: >> + $ref: /schemas/graph.yaml#/$defs/port-base >> + unevaluatedProperties: false >> + description: CSI-2 input port >> + >> + properties: >> + endpoint: >> + $ref: /schemas/media/video-interfaces.yaml# >> + unevaluatedProperties: false > > Should the data-lanes property be required for the CSI-2 input ? Yes. >> + >> + port@1: >> + $ref: /schemas/graph.yaml#/properties/port >> + unevaluatedProperties: false >> + description: FPD-Link 3 output port >> + >> + i2c: >> + $ref: /schemas/i2c/i2c-controller.yaml# >> + unevaluatedProperties: false >> + >> +required: >> + - compatible >> + - '#gpio-cells' >> + - gpio-controller >> + - '#clock-cells' >> + - ports >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/gpio/gpio.h> >> + >> + serializer { >> + compatible = "ti,ds90ub953-q1"; >> + >> + gpio-controller; >> + #gpio-cells = <2>; >> + >> + #clock-cells = <0>; >> + >> + ports { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + port@0 { >> + reg = <0>; >> + ub953_in: endpoint { >> + clock-lanes = <0>; >> + data-lanes = <1 2 3 4>; >> + remote-endpoint = <&sensor_out>; >> + }; >> + }; >> + >> + port@1 { >> + reg = <1>; >> + endpoint { >> + remote-endpoint = <&deser_fpd_in>; >> + }; >> + }; >> + }; >> + >> + i2c { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + sensor@1a { >> + compatible = "sony,imx274"; >> + reg = <0x1a>; >> + >> + reset-gpios = <&serializer 0 GPIO_ACTIVE_LOW>; > > Maybe add > > clocks = <&serializer>; > clock-names = "inck"; > > to showcase the clock connection ? Yes, that's a good idea. Tomi
Hi Rob, On 09/12/2022 23:27, Rob Herring wrote: > On Thu, Dec 08, 2022 at 12:40:02PM +0200, Tomi Valkeinen wrote: >> Add DT bindings for TI DS90UB953 FPDLink-3 Serializer. > > Seems like this and DS90UB913 binding could be combined. I couldn't spot > a difference. They are indeed quite similar, but there are a few diffs, especially after fixing Laurent's review comments. E.g, as the UB913 is a parallel video serializer and the UB953 is a CSI-2 serializer, the input port on UB913 has 'pclk-sample' property but UB953 has 'data-lanes' property. The descriptions differ also a bit for the above mentioned difference. The above points would still allow combining the bindings, though. But I feel the UB913 is somewhat a different class of serializer device compared to UB953 (and UB971 which the UB953's binding also supports), so my gut feeling says it's better to keep them separate. But I don't have much experience on maintaining such bindings, and, afaik, we could always split the bindings later if needed. So... Do you have a preference on either way? Or maybe we can come back to this after I send the next version with the updates. > In the subjects, drop 'binding for'. The prefix says this is a binding. > Maybe add 'Serializer'. Ok. Tomi
diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953.yaml new file mode 100644 index 000000000000..fd7d25d93e2c --- /dev/null +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953.yaml @@ -0,0 +1,112 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/ti,ds90ub953.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Texas Instruments DS90UB953 FPD-Link 3 Serializer + +maintainers: + - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> + +description: + The TI DS90UB953 is an FPD-Link 3 video serializer for MIPI CSI-2. + +properties: + compatible: + enum: + - ti,ds90ub953-q1 + - ti,ds90ub971-q1 + + '#gpio-cells': + const: 2 + + gpio-controller: true + + '#clock-cells': + const: 0 + + ports: + $ref: /schemas/graph.yaml#/properties/ports + + properties: + port@0: + $ref: /schemas/graph.yaml#/$defs/port-base + unevaluatedProperties: false + description: CSI-2 input port + + properties: + endpoint: + $ref: /schemas/media/video-interfaces.yaml# + unevaluatedProperties: false + + port@1: + $ref: /schemas/graph.yaml#/properties/port + unevaluatedProperties: false + description: FPD-Link 3 output port + + i2c: + $ref: /schemas/i2c/i2c-controller.yaml# + unevaluatedProperties: false + +required: + - compatible + - '#gpio-cells' + - gpio-controller + - '#clock-cells' + - ports + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + + serializer { + compatible = "ti,ds90ub953-q1"; + + gpio-controller; + #gpio-cells = <2>; + + #clock-cells = <0>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + ub953_in: endpoint { + clock-lanes = <0>; + data-lanes = <1 2 3 4>; + remote-endpoint = <&sensor_out>; + }; + }; + + port@1 { + reg = <1>; + endpoint { + remote-endpoint = <&deser_fpd_in>; + }; + }; + }; + + i2c { + #address-cells = <1>; + #size-cells = <0>; + + sensor@1a { + compatible = "sony,imx274"; + reg = <0x1a>; + + reset-gpios = <&serializer 0 GPIO_ACTIVE_LOW>; + + port { + sensor_out: endpoint { + remote-endpoint = <&ub953_in>; + }; + }; + }; + }; + }; +...
Add DT bindings for TI DS90UB953 FPDLink-3 Serializer. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- .../bindings/media/i2c/ti,ds90ub953.yaml | 112 ++++++++++++++++++ 1 file changed, 112 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub953.yaml