Re: [PATCH 1/4] dt-bindings: display: bridge: tfp410: Add tfp410 i2c example

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Jon Cormier <jcormier@criticallink.com>
Cc: Andrzej Hajda <andrzej.hajda@intel.com>,
	Neil Armstrong <neil.armstrong@linaro.org>,
	Robert Foss <robert.foss@linaro.org>,
	Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
	Jonas Karlman <jonas@kwiboo.se>,
	Jernej Skrabec <jernej.skrabec@gmail.com>,
	David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Tomi Valkeinen <tomi.valkeinen@ti.com>,
	Jyri Sarha <jsarha@ti.com>,
	dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Michael Williamson <michael.williamson@criticallink.com>,
	Bob Duke <bduke@criticallink.com>
Subject: Re: [PATCH 1/4] dt-bindings: display: bridge: tfp410: Add tfp410 i2c example
Date: Fri, 27 Jan 2023 09:30:30 +0100	[thread overview]
Message-ID: <2e183f7b-df8a-38c6-a041-d507fc32e894@linaro.org> (raw)
In-Reply-To: <CADL8D3YUNnsZt8tc8x9CxH5Ug6kWJHb=a3N5VJFPSePWH3yWxg@mail.gmail.com>

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