Re: [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 bindings

[Marek Vasut <marex@denx.de>]

On 2/4/21 6:15 PM, Doug Anderson wrote:

Hi,

[...]

>> +properties:
>> +  compatible:
>> +    const: ti,sn65dsi83
>> +
>> +  reg:
>> +    const: 0x2d
>> +
>> +  enable-gpios:
>> +    maxItems: 1
>> +    description: GPIO specifier for bridge_en pin (active high).
> 
> I see two regulators: vcc and vcore.  Shouldn't those be listed?

Those are not implemented and not tested, so if someone needs them later 
on, they can be added then.

> I also see an interrupt pin on the datasheet.  Probably should be
> listed too even if the chip can be made to work fine without hooking
> it up.  It can just be optional, right?

It is optional and again completely untested, so it can be added later 
if needed.

> It wouldn't hurt to list the refclk here too even if the code doesn't
> use it.  From sn65dsi86 it was handy that the bindings already had all
> this type of stuff so that when we added the feature we didn't have to
> go back to the bindings.

In my case, the refclock are derived from the DSI link.

>> +  ports:
>> +    type: object
>> +    additionalProperties: false
>> +
>> +    properties:
>> +      "#address-cells":
>> +        const: 1
>> +
>> +      "#size-cells":
>> +        const: 0
>> +
>> +      port@0:
>> +        type: object
>> +        additionalProperties: false
>> +
>> +        description:
>> +          Video port for MIPI DSI input
>> +
>> +        properties:
>> +          reg:
>> +            const: 0
>> +
>> +          endpoint:
>> +            type: object
>> +            additionalProperties: false
>> +            properties:
>> +              remote-endpoint: true
>> +              data-lanes:
>> +                description: array of physical DSI data lane indexes.
> 
> The chip doesn't allow for arbitrary remapping here, right?  So you're
> just using this as the official way to specify the number of lanes?  I
> guess the only valid values are:
> 
> <0>
> <0 1>
> <0 1 2>
> <0 1 2 3>

Shouldn't that be <1 2 3 4> ?

> In sn65dsi86 we attempted to enforce that a valid option was selected
> for the output lanes.  Could you do something similar?  If nothing
> else adding a description of the valid options would be good.

I saw the binding, but I was under the impressions the DSI86 can do lane 
reordering, isn't that the case ? Maybe I misunderstood it.

But yes, if you have a suggestion how to make a non-cryptic list of 
those four lane mapping options, please do share this info.

>> +        required:
>> +          - reg
>> +
>> +      port@1:
>> +        type: object
>> +        additionalProperties: false
>> +
>> +        description:
>> +          Video port for LVDS output (panel or bridge).
>> +
>> +        properties:
>> +          reg:
>> +            const: 1
>> +
>> +          endpoint:
>> +            type: object
>> +            additionalProperties: false
>> +            properties:
>> +              remote-endpoint: true
> 
> Worth adding the data-lanes here too?  I guess this part allows you
> two different orders for the LVDS outputs?

I don't really want to add any properties which I cannot test and then 
end up with DT bindings which would become poor ABI in the future.

>> +
>> +        required:
>> +          - reg
>> +
>> +    required:
>> +      - "#address-cells"
>> +      - "#size-cells"
>> +      - port@0
>> +      - port@1
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - enable-gpios
>> +  - ports
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/gpio/gpio.h>
>> +
>> +    i2c {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      bridge@2d {
>> +        compatible = "ti,sn65dsi83";
>> +        reg = <0x2d>;
>> +
>> +        enable-gpios = <&gpio2 1 GPIO_ACTIVE_HIGH>;
>> +
>> +        ports {
>> +          #address-cells = <1>;
>> +          #size-cells = <0>;
>> +
>> +          port@0 {
>> +            reg = <0>;
>> +            endpoint {
>> +              remote-endpoint = <&dsi0_out>;
>> +              data-lanes = <1 2 3 4>;
> 
> Should the above be <0 1 2 3>?

Well, git grep data-lanes seems to indicate some count from 1, some from 0 .