Re: [PATCH 1/7] dt-bindings: display/msm/dsi: allow specifying TE source

[Abhinav Kumar <quic_abhinavk@quicinc.com>]

On 5/22/2024 1:05 PM, Dmitry Baryshkov wrote:
> On Wed, 22 May 2024 at 21:38, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 5/20/2024 5:12 AM, Dmitry Baryshkov wrote:
>>> Command mode panels provide TE signal back to the DSI host to signal
>>> that the frame display has completed and update of the image will not
>>> cause tearing. Usually it is connected to the first GPIO with the
>>> mdp_vsync function, which is the default. In such case the property can
>>> be skipped.
>>>
>>
>> This is a good addition overall. Some comments below.
>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>    .../bindings/display/msm/dsi-controller-main.yaml        | 16 ++++++++++++++++
>>>    1 file changed, 16 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
>>> index 1fa28e976559..c1771c69b247 100644
>>> --- a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
>>> +++ b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
>>> @@ -162,6 +162,21 @@ properties:
>>>                    items:
>>>                      enum: [ 0, 1, 2, 3 ]
>>>
>>> +              qcom,te-source:
>>> +                $ref: /schemas/types.yaml#/definitions/string
>>> +                description:
>>> +                  Specifies the source of vsync signal from the panel used for
>>> +                  tearing elimination. The default is mdp_gpio0.
>>
>> panel --> command mode panel?
>>
>>> +                enum:
>>> +                  - mdp_gpio0
>>> +                  - mdp_gpio1
>>> +                  - mdp_gpio2
>>
>> are gpio0, gpio1 and gpio2 referring to the vsync_p, vsync_s and vsync_e
>> sources?
> 
> No idea, unfortunately. They are gpioN or just mdp_vsync all over the
> place. For the reference, in case of the SDM845 and Pixel3 the signal
> is routed through SoC GPIO12.
> 

GPIO12 on sdm845 is mdp_vsync_e.

Thats why I think its better we use mdp_vsync_p/s/e instead of mdp_gpio0/1/2

>> In that case wouldnt it be better to name it like that?
>>
>>> +                  - timer0
>>> +                  - timer1
>>> +                  - timer2
>>> +                  - timer3
>>> +                  - timer4
>>> +
>>
>> These are indicating watchdog timer sources right?
> 
> Yes.
> 
>>
>>>        required:
>>>          - port@0
>>>          - port@1
>>> @@ -452,6 +467,7 @@ examples:
>>>                              dsi0_out: endpoint {
>>>                                       remote-endpoint = <&sn65dsi86_in>;
>>>                                       data-lanes = <0 1 2 3>;
>>> +                                   qcom,te-source = "mdp_gpio2";
>>
>> I have a basic doubt on this. Should te-source should be in the input
>> port or the output one for the controller? Because TE is an input to the
>> DSI. And if the source is watchdog timer then it aligns even more as a
>> property of the input endpoint.
> 
> I don't really want to split this. Both data-lanes and te-source are
> properties of the link between the DSI and panel. You can not really
> say which side has which property.
> 

TE is an input to the DSI from the panel. Between input and output port, 
I think it belongs more to the input port.

I didnt follow why this is a link property. Sorry , I didnt follow the 
split part.

If we are unsure about input vs output port, do you think its better we 
make it a property of the main dsi node instead?

>>>                              };
>>>                      };
>>>               };
>>>
>