| Message ID | 20240305152608.287527-3-julien.massot@collabora.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for MAX96714F and MAX96717F GMSL2 ser/des | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Tue, Mar 05, 2024 at 04:26:06PM +0100, Julien Massot wrote: > Add DT bindings for Maxim MAX96714 GMSL2 Deserializer. > > Signed-off-by: Julien Massot <julien.massot@collabora.com> > --- > Change since v3: > - Renamed file to maxim,max96714.yaml dropped the 'f' suffix Why? The filename should match the compatible, which /does/ have an f. > - Removed mention to C-PHY since it's not supported by MAX96714 deserializers > - Removed bus-type requirement on CSI endpoint since the device only support D-PHY > - Removed the clock-lanes property in the dt example > > Change since v2: > - remove reg description > - rename enable gpio to powerdown > - use generic node name: i2c, serializer, deserializer > --- > --- > .../bindings/media/i2c/maxim,max96714.yaml | 169 ++++++++++++++++++ > 1 file changed, 169 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/maxim,max96714.yaml > +properties: > + compatible: > + const: maxim,max96714f > + i2c-gate: > + $ref: /schemas/i2c/i2c-controller.yaml There is an i2c-gate binding, you should reference it here instead. > + unevaluatedProperties: false > + description: | This | is not needed, there's no formatting to preserve. > + The MAX96714 will pass through and forward the I2C requests from the > + incoming I2C bus over the GMSL2 link. Therefore it supports an i2c-gate > + subnode to configure a serializer.
Hi Conor, Thanks for reviewing my patchset. On 3/7/24 20:21, Conor Dooley wrote: > On Tue, Mar 05, 2024 at 04:26:06PM +0100, Julien Massot wrote: >> Add DT bindings for Maxim MAX96714 GMSL2 Deserializer. >> >> Signed-off-by: Julien Massot <julien.massot@collabora.com> >> --- >> Change since v3: >> - Renamed file to maxim,max96714.yaml dropped the 'f' suffix > > Why? The filename should match the compatible, which /does/ have an f. All the work has been done on MAX96714F variant of this Maxim GMSL2 deserializer. The driver and the binding remain suitable for all variants of this chipset, since they share the same register mapping, similar features etc.. MAX96714 exists in different variant: MAX96714 / MAX96714F / MAX96714K that will be easy to add support for this binding and driver later. The MAX96714 name looks the most suitable. Please have a look at this discussion on the V3 version https://lore.kernel.org/lkml/ZdXYpc2csVnhtZH9@valkosipuli.retiisi.eu > >> - Removed mention to C-PHY since it's not supported by MAX96714 deserializers >> - Removed bus-type requirement on CSI endpoint since the device only support D-PHY >> - Removed the clock-lanes property in the dt example >> >> Change since v2: >> - remove reg description >> - rename enable gpio to powerdown >> - use generic node name: i2c, serializer, deserializer >> --- >> --- >> .../bindings/media/i2c/maxim,max96714.yaml | 169 ++++++++++++++++++ >> 1 file changed, 169 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/media/i2c/maxim,max96714.yaml > >> +properties: >> + compatible: >> + const: maxim,max96714f > >> + i2c-gate: >> + $ref: /schemas/i2c/i2c-controller.yaml > > There is an i2c-gate binding, you should reference it here instead. Ok, I will post a new version with a reference to the i2c-gate binding. > >> + unevaluatedProperties: false >> + description: | > > This | is not needed, there's no formatting to preserve. Ok I will drop the '|' > >> + The MAX96714 will pass through and forward the I2C requests from the >> + incoming I2C bus over the GMSL2 link. Therefore it supports an i2c-gate >> + subnode to configure a serializer. Regards,
On Fri, Mar 08, 2024 at 03:08:12PM +0100, Julien Massot wrote: > On 3/7/24 20:21, Conor Dooley wrote: > > On Tue, Mar 05, 2024 at 04:26:06PM +0100, Julien Massot wrote: > > > Add DT bindings for Maxim MAX96714 GMSL2 Deserializer. > > > > > > Signed-off-by: Julien Massot <julien.massot@collabora.com> > > > --- > > > Change since v3: > > > - Renamed file to maxim,max96714.yaml dropped the 'f' suffix > > > > Why? The filename should match the compatible, which /does/ have an f. > All the work has been done on MAX96714F variant of this Maxim GMSL2 > deserializer. > The driver and the binding remain suitable for all variants of this chipset, > since they share the same > register mapping, similar features etc.. > > MAX96714 exists in different variant: MAX96714 / MAX96714F / MAX96714K that > will be easy > to add support for this binding and driver later. Either document the non-f version if it really is that similar, using all of the same properties, or name the file after the version you've actually documented. I don't see why this particular case should be given an exception to how bindings are named. What is the actual difference between the f and non f versions? Is it visible to software? > The MAX96714 name looks the most suitable. > Please have a look at this discussion on the V3 version > https://lore.kernel.org/lkml/ZdXYpc2csVnhtZH9@valkosipuli.retiisi.eu
On 3/8/24 16:07, Conor Dooley wrote: > On Fri, Mar 08, 2024 at 03:08:12PM +0100, Julien Massot wrote: >> On 3/7/24 20:21, Conor Dooley wrote: >>> On Tue, Mar 05, 2024 at 04:26:06PM +0100, Julien Massot wrote: >>>> Add DT bindings for Maxim MAX96714 GMSL2 Deserializer. >>>> >>>> Signed-off-by: Julien Massot <julien.massot@collabora.com> >>>> --- >>>> Change since v3: >>>> - Renamed file to maxim,max96714.yaml dropped the 'f' suffix >>> >>> Why? The filename should match the compatible, which /does/ have an f. >> All the work has been done on MAX96714F variant of this Maxim GMSL2 >> deserializer. >> The driver and the binding remain suitable for all variants of this chipset, >> since they share the same >> register mapping, similar features etc.. >> >> MAX96714 exists in different variant: MAX96714 / MAX96714F / MAX96714K that >> will be easy >> to add support for this binding and driver later. > > Either document the non-f version if it really is that similar, using > all of the same properties, or name the file after the version you've > actually documented. I don't see why this particular case should be > given an exception to how bindings are named. > > What is the actual difference between the f and non f versions? Is it > visible to software? Yes there are a few differences visible to the software, for example the GMSL link rate since MAX96714 support 6 and 3 Gbps, while MAX96714F only supports 3Gbps. the registers map is the same, but a few values are not possible with the 'f' version. I will add a compatible for the non 'f' version, and will do the same for the max96717 binding.
On Fri, Mar 08, 2024 at 04:48:16PM +0100, Julien Massot wrote: > > > On 3/8/24 16:07, Conor Dooley wrote: > > On Fri, Mar 08, 2024 at 03:08:12PM +0100, Julien Massot wrote: > > > On 3/7/24 20:21, Conor Dooley wrote: > > > > On Tue, Mar 05, 2024 at 04:26:06PM +0100, Julien Massot wrote: > > > > > Add DT bindings for Maxim MAX96714 GMSL2 Deserializer. > > > > > > > > > > Signed-off-by: Julien Massot <julien.massot@collabora.com> > > > > > --- > > > > > Change since v3: > > > > > - Renamed file to maxim,max96714.yaml dropped the 'f' suffix > > > > > > > > Why? The filename should match the compatible, which /does/ have an f. > > > All the work has been done on MAX96714F variant of this Maxim GMSL2 > > > deserializer. > > > The driver and the binding remain suitable for all variants of this chipset, > > > since they share the same > > > register mapping, similar features etc.. > > > > > > MAX96714 exists in different variant: MAX96714 / MAX96714F / MAX96714K that > > > will be easy > > > to add support for this binding and driver later. > > > > Either document the non-f version if it really is that similar, using > > all of the same properties, or name the file after the version you've > > actually documented. I don't see why this particular case should be > > given an exception to how bindings are named. > > > > What is the actual difference between the f and non f versions? Is it > > visible to software? > > Yes there are a few differences visible to the software, for example the > GMSL > link rate since MAX96714 support 6 and 3 Gbps, while MAX96714F only supports > 3Gbps. > the registers map is the same, but a few values are not possible with the > 'f' version. > > I will add a compatible for the non 'f' version, and will do the same for > the max96717 binding. It's not immediately clear if that means that the f version should be a fallback for the non-f version, but sounds like it could be if the difference is purely that there's a reduced set of link rates or similar.
diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max96714.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max96714.yaml new file mode 100644 index 000000000000..d76a5213ef0f --- /dev/null +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max96714.yaml @@ -0,0 +1,169 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +# Copyright (C) 2024 Collabora Ltd. +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/maxim,max96714.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Maxim MAX96714 GMSL2 to CSI-2 Deserializer + +maintainers: + - Julien Massot <julien.massot@collabora.com> + +description: | + The MAX96714F deserializer converts GMSL2 serial inputs into MIPI + CSI-2 D-PHY formatted output. The device allows the GMSL2 link to + simultaneously transmit bidirectional control-channel data while forward + video transmissions are in progress. The MAX96714F can connect to one + remotely located serializer using industry-standard coax or STP + interconnects. The device cans operate in pixel or tunnel mode. In pixel mode + the MAX96714F can select individual video stream, while the tunnel mode forward all + the MIPI data received by the serializer. + + The GMSL2 serial link operates at a fixed rate of 3Gbps in the + forward direction and 187.5Mbps in the reverse direction. + +properties: + compatible: + const: maxim,max96714f + + reg: + maxItems: 1 + + powerdown-gpios: + maxItems: 1 + description: + Specifier for the GPIO connected to the PWDNB pin. + + ports: + $ref: /schemas/graph.yaml#/properties/ports + + properties: + port@0: + $ref: /schemas/graph.yaml#/properties/port + unevaluatedProperties: false + description: GMSL Input + properties: + endpoint: + $ref: /schemas/media/video-interfaces.yaml# + unevaluatedProperties: false + description: + Endpoint for GMSL2-Link port. + + port@1: + $ref: /schemas/graph.yaml#/$defs/port-base + unevaluatedProperties: false + description: CSI-2 Output port + + properties: + endpoint: + $ref: /schemas/media/video-interfaces.yaml# + unevaluatedProperties: false + + properties: + data-lanes: + minItems: 1 + maxItems: 4 + + lane-polarities: + minItems: 1 + maxItems: 5 + + link-frequencies: + maxItems: 1 + + required: + - data-lanes + + required: + - port@1 + + i2c-gate: + $ref: /schemas/i2c/i2c-controller.yaml + unevaluatedProperties: false + description: | + The MAX96714 will pass through and forward the I2C requests from the + incoming I2C bus over the GMSL2 link. Therefore it supports an i2c-gate + subnode to configure a serializer. + + port0-poc-supply: + description: Regulator providing Power over Coax for the GMSL port + +required: + - compatible + - reg + - ports + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/media/video-interfaces.h> + + i2c { + #address-cells = <1>; + #size-cells = <0>; + + deserializer@28 { + compatible = "maxim,max96714f"; + reg = <0x28>; + powerdown-gpios = <&main_gpio0 37 GPIO_ACTIVE_LOW>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + port@0 { + reg = <0>; + max96714_gmsl_in: endpoint { + remote-endpoint = <&max96917f_gmsl_out>; + }; + }; + + port@1 { + reg = <1>; + max96714_csi_out: endpoint { + data-lanes = <1 2 3 4>; + link-frequencies = /bits/ 64 <400000000>; + remote-endpoint = <&csi_in>; + }; + }; + }; + + i2c-gate { + #address-cells = <1>; + #size-cells = <0>; + + serializer@40 { + compatible = "maxim,max96717f"; + reg = <0x40>; + gpio-controller; + #gpio-cells = <2>; + #clock-cells = <0>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + max96717f_csi_in: endpoint { + bus-type = <MEDIA_BUS_TYPE_CSI2_DPHY>; + data-lanes = <1 2>; + lane-polarities = <1 0 1>; + remote-endpoint = <&sensor_out>; + }; + }; + + port@1 { + reg = <1>; + max96917f_gmsl_out: endpoint { + remote-endpoint = <&max96714_gmsl_in>; + }; + }; + }; + }; + }; + }; + }; +...
Add DT bindings for Maxim MAX96714 GMSL2 Deserializer. Signed-off-by: Julien Massot <julien.massot@collabora.com> --- Change since v3: - Renamed file to maxim,max96714.yaml dropped the 'f' suffix - Removed mention to C-PHY since it's not supported by MAX96714 deserializers - Removed bus-type requirement on CSI endpoint since the device only support D-PHY - Removed the clock-lanes property in the dt example Change since v2: - remove reg description - rename enable gpio to powerdown - use generic node name: i2c, serializer, deserializer --- --- .../bindings/media/i2c/maxim,max96714.yaml | 169 ++++++++++++++++++ 1 file changed, 169 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/maxim,max96714.yaml