| Message ID | 20230125-tfp410_i2c-v1-1-66a4d4e390b7@criticallink.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DRM: BRIDGE: TFP410: Add i2c support | expand |
Hi Jonathan, Thank you for the patch. On Wed, Jan 25, 2023 at 04:09:09PM -0500, Jonathan Cormier wrote: > Add a i2c example with HDMI connector > > Signed-off-by: Jonathan Cormier <jcormier@criticallink.com> > --- > .../bindings/display/bridge/ti,tfp410.yaml | 42 ++++++++++++++++++++++ > 1 file changed, 42 insertions(+) > > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml > index 4c5dd8ec2951..456214f14b47 100644 > --- a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml > +++ b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml > @@ -116,4 +116,46 @@ examples: > }; > }; > > + - | > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; Please use 4 spaces for indentation, as in the other example. > + > + hdmi_encoder: tfp410@38 { > + compatible = "ti,tfp410"; > + reg = <0x38>; > + > + ports { > + address-cells = <1>; > + size-cells = <0>; > + > + port@0 { > + reg = <0>; > + tfp410_in: endpoint { > + remote-endpoint = <&dpi1_out>; > + }; > + }; > + > + port@1 { > + reg = <1>; > + tfp410_out: endpoint { > + remote-endpoint = <&hdmi_connector_in>; > + }; > + }; > + }; > + }; > + }; > + > + hdmi: hdmi_connector { > + compatible = "hdmi-connector"; > + label = "hdmi"; > + type = "a"; > + ddc-i2c-bus = <&i2c1>; > + port { > + hdmi_connector_in: endpoint { > + remote-endpoint = <&tfp410_out>; > + }; > + }; > + }; > + You can drop the hdmi connector, the example will still validate. > ...
On Wed, Jan 25, 2023 at 4:24 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Jonathan, > > Thank you for the patch. > > On Wed, Jan 25, 2023 at 04:09:09PM -0500, Jonathan Cormier wrote: > > Add a i2c example with HDMI connector > > > > Signed-off-by: Jonathan Cormier <jcormier@criticallink.com> > > --- > > .../bindings/display/bridge/ti,tfp410.yaml | 42 ++++++++++++++++++++++ > > 1 file changed, 42 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml > > index 4c5dd8ec2951..456214f14b47 100644 > > --- a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml > > +++ b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml > > @@ -116,4 +116,46 @@ examples: > > }; > > }; > > > > + - | > > + i2c { > > + #address-cells = <1>; > > + #size-cells = <0>; > > Please use 4 spaces for indentation, as in the other example. Will do, the whole file is 2 space indents. I didn't notice the examples switch to 4 spaces. > > > + > > + hdmi_encoder: tfp410@38 { > > + compatible = "ti,tfp410"; > > + reg = <0x38>; > > + > > + ports { > > + address-cells = <1>; > > + size-cells = <0>; > > + > > + port@0 { > > + reg = <0>; > > + tfp410_in: endpoint { > > + remote-endpoint = <&dpi1_out>; > > + }; > > + }; > > + > > + port@1 { > > + reg = <1>; > > + tfp410_out: endpoint { > > + remote-endpoint = <&hdmi_connector_in>; > > + }; > > + }; > > + }; > > + }; > > + }; > > + > > + hdmi: hdmi_connector { > > + compatible = "hdmi-connector"; > > + label = "hdmi"; > > + type = "a"; > > + ddc-i2c-bus = <&i2c1>; > > + port { > > + hdmi_connector_in: endpoint { > > + remote-endpoint = <&tfp410_out>; > > + }; > > + }; > > + }; > > + > > You can drop the hdmi connector, the example will still validate. Okay > > > ... > > -- > Regards, > > Laurent Pinchart
On Wed, 25 Jan 2023 16:09:09 -0500, Jonathan Cormier wrote: > Add a i2c example with HDMI connector > > Signed-off-by: Jonathan Cormier <jcormier@criticallink.com> > --- > .../bindings/display/bridge/ti,tfp410.yaml | 42 ++++++++++++++++++++++ > 1 file changed, 42 insertions(+) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: Documentation/devicetree/bindings/display/bridge/ti,tfp410.example.dts:77.37-79.19: ERROR (duplicate_label): /example-1/i2c/tfp410@38/ports/port@0/endpoint: Duplicate label 'tfp410_in' on /example-1/i2c/tfp410@38/ports/port@0/endpoint and /example-0/encoder/ports/port@0/endpoint Documentation/devicetree/bindings/display/bridge/ti,tfp410.example.dts:84.38-86.19: ERROR (duplicate_label): /example-1/i2c/tfp410@38/ports/port@1/endpoint: Duplicate label 'tfp410_out' on /example-1/i2c/tfp410@38/ports/port@1/endpoint and /example-0/encoder/ports/port@1/endpoint ERROR: Input tree has errors, aborting (use -f to force output) make[1]: *** [scripts/Makefile.lib:434: Documentation/devicetree/bindings/display/bridge/ti,tfp410.example.dtb] Error 2 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:1508: dt_binding_check] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230125-tfp410_i2c-v1-1-66a4d4e390b7@criticallink.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On 25/01/2023 22:09, Jonathan Cormier wrote: > Add a i2c example with HDMI connector Why? It's the same - but more on this below. > > Signed-off-by: Jonathan Cormier <jcormier@criticallink.com> You need to test the bindings before sending and fix the errors. > --- > .../bindings/display/bridge/ti,tfp410.yaml | 42 ++++++++++++++++++++++ > 1 file changed, 42 insertions(+) > > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml > index 4c5dd8ec2951..456214f14b47 100644 > --- a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml > +++ b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml > @@ -116,4 +116,46 @@ examples: > }; > }; > > + - | > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + hdmi_encoder: tfp410@38 { Node names should be generic. https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > + compatible = "ti,tfp410"; > + reg = <0x38>; > + > + ports { > + address-cells = <1>; > + size-cells = <0>; > + > + port@0 { > + reg = <0>; > + tfp410_in: endpoint { > + remote-endpoint = <&dpi1_out>; > + }; > + }; > + > + port@1 { > + reg = <1>; > + tfp410_out: endpoint { > + remote-endpoint = <&hdmi_connector_in>; > + }; That's the same example as existing one, so it looks useless. I don't see benefits of this example. > + }; > + }; > + }; > + }; > + > + hdmi: hdmi_connector { Drop. Incorrect name and not really related. > + compatible = "hdmi-connector"; > + label = "hdmi"; > + type = "a"; > + ddc-i2c-bus = <&i2c1>; > + port { > + hdmi_connector_in: endpoint { > + remote-endpoint = <&tfp410_out>; > + }; > + }; > + }; > + > ... > Best regards, Krzysztof
On Thu, Jan 26, 2023 at 10:40 AM Krzysztof Kozlowski < krzysztof.kozlowski@linaro.org> wrote: > On 25/01/2023 22:09, Jonathan Cormier wrote: > > Add a i2c example with HDMI connector > > Why? It's the same - but more on this below. > The existing example is for the previous setup where it was configured as its own device. It seemed necessary now that the driver is going to support being connected to an i2c bus to show it being used as such. > > > > > Signed-off-by: Jonathan Cormier <jcormier@criticallink.com> > > You need to test the bindings before sending and fix the errors. > Will do > > > --- > > .../bindings/display/bridge/ti,tfp410.yaml | 42 > ++++++++++++++++++++++ > > 1 file changed, 42 insertions(+) > > > > diff --git > a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml > b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml > > index 4c5dd8ec2951..456214f14b47 100644 > > --- a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml > > +++ b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml > > @@ -116,4 +116,46 @@ examples: > > }; > > }; > > > > + - | > > + i2c { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + hdmi_encoder: tfp410@38 { > > Node names should be generic. > > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > > Can do > > + compatible = "ti,tfp410"; > > + reg = <0x38>; > > + > > + ports { > > + address-cells = <1>; > > + size-cells = <0>; > > + > > + port@0 { > > + reg = <0>; > > + tfp410_in: endpoint { > > + remote-endpoint = <&dpi1_out>; > > + }; > > + }; > > + > > + port@1 { > > + reg = <1>; > > + tfp410_out: endpoint { > > + remote-endpoint = <&hdmi_connector_in>; > > + }; > > That's the same example as existing one, so it looks useless. I don't > see benefits of this example. > It's mostly the same, except defined inside an i2c bus, with the reg value set. Without the powerdown-gpios or ti,deskew. And without the pclk-sample and bus-width (these are now read from i2c) And I included the hdmi_connector so it would be a more complete and useful example of how it could be used. The TFP410 doesn't handle the ddc i2c bus on its own so a separate connector node is needed. I'll drop it if that's preferred. > > > + }; > > + }; > > + }; > > + }; > > + > > + hdmi: hdmi_connector { > > Drop. Incorrect name and not really related. > > > + compatible = "hdmi-connector"; > > + label = "hdmi"; > > + type = "a"; > > + ddc-i2c-bus = <&i2c1>; > > + port { > > + hdmi_connector_in: endpoint { > > + remote-endpoint = <&tfp410_out>; > > + }; > > + }; > > + }; > > + > > ... > > > > Best regards, > Krzysztof > >
On 26/01/2023 19:36, Jon Cormier wrote: > On Thu, Jan 26, 2023 at 10:40 AM Krzysztof Kozlowski < > krzysztof.kozlowski@linaro.org> wrote: > >> On 25/01/2023 22:09, Jonathan Cormier wrote: >>> Add a i2c example with HDMI connector >> >> Why? It's the same - but more on this below. >> > The existing example is for the previous setup where it was configured as > its own device. It seemed necessary now that the driver is going to > support being connected to an i2c bus to show it being used as such. > >> >>> >>> Signed-off-by: Jonathan Cormier <jcormier@criticallink.com> >> >> You need to test the bindings before sending and fix the errors. >> > Will do > >> >>> --- >>> .../bindings/display/bridge/ti,tfp410.yaml | 42 >> ++++++++++++++++++++++ >>> 1 file changed, 42 insertions(+) >>> >>> diff --git >> a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml >> b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml >>> index 4c5dd8ec2951..456214f14b47 100644 >>> --- a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml >>> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml >>> @@ -116,4 +116,46 @@ examples: >>> }; >>> }; >>> >>> + - | >>> + i2c { >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + >>> + hdmi_encoder: tfp410@38 { >> >> Node names should be generic. >> >> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation >> >> Can do > >>> + compatible = "ti,tfp410"; >>> + reg = <0x38>; >>> + >>> + ports { >>> + address-cells = <1>; >>> + size-cells = <0>; >>> + >>> + port@0 { >>> + reg = <0>; >>> + tfp410_in: endpoint { >>> + remote-endpoint = <&dpi1_out>; >>> + }; >>> + }; >>> + >>> + port@1 { >>> + reg = <1>; >>> + tfp410_out: endpoint { >>> + remote-endpoint = <&hdmi_connector_in>; >>> + }; >> >> That's the same example as existing one, so it looks useless. I don't >> see benefits of this example. >> > It's mostly the same, except defined inside an i2c bus, with the reg value > set. Without the powerdown-gpios or ti,deskew. > And without the pclk-sample and bus-width (these are now read from i2c) > And I included the hdmi_connector so it would be a more complete and useful > example of how it could be used. hdmi_connector is being dropped because it is not related. > The TFP410 doesn't handle the ddc i2c bus > on its own so a separate connector node is needed. I'll drop it if that's > preferred. > If you had here different ports, it would be different case. But as of now the only important part is having reg and not having gpios, so basically almost the same example. No need for it. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml index 4c5dd8ec2951..456214f14b47 100644 --- a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml +++ b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml @@ -116,4 +116,46 @@ examples: }; }; + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + hdmi_encoder: tfp410@38 { + compatible = "ti,tfp410"; + reg = <0x38>; + + ports { + address-cells = <1>; + size-cells = <0>; + + port@0 { + reg = <0>; + tfp410_in: endpoint { + remote-endpoint = <&dpi1_out>; + }; + }; + + port@1 { + reg = <1>; + tfp410_out: endpoint { + remote-endpoint = <&hdmi_connector_in>; + }; + }; + }; + }; + }; + + hdmi: hdmi_connector { + compatible = "hdmi-connector"; + label = "hdmi"; + type = "a"; + ddc-i2c-bus = <&i2c1>; + port { + hdmi_connector_in: endpoint { + remote-endpoint = <&tfp410_out>; + }; + }; + }; + ...
Add a i2c example with HDMI connector Signed-off-by: Jonathan Cormier <jcormier@criticallink.com> --- .../bindings/display/bridge/ti,tfp410.yaml | 42 ++++++++++++++++++++++ 1 file changed, 42 insertions(+)