Re: [PATCH v11 3/5] dt-bindings: drm/bridge: Document sn65dsi86 bridge bindings

[spanda-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org]

On 2018-06-15 12:53, Andrzej Hajda wrote:
> On 15.06.2018 08:43, Sandeep Panda wrote:
>> Document the bindings used for the sn65dsi86 DSI to eDP bridge.
>> 
>> Changes in v1:
>>  - Rephrase the dt-binding descriptions to be more inline with 
>> existing
>>    bindings (Andrzej Hajda).
>>  - Add missing dt-binding that are parsed by corresponding driver
>>    (Andrzej Hajda).
>> 
>> Changes in v2:
>>  - Remove edp panel specific dt-binding entries. Only keep bridge
>>    specific entries (Sean Paul).
>>  - Remove custom-modes dt entry since its usage is removed from driver 
>> also (Sean Paul).
>>  - Remove is-pluggable dt entry since this will not be needed anymore 
>> (Sean Paul).
>> 
>> Changes in v3:
>>  - Remove irq-gpio dt entry and instead populate is an interrupt
>>    property (Rob Herring).
>> 
>> Changes in v4:
>>  - Add link to bridge chip datasheet (Stephen Boyd)
>>  - Add vpll and vcc regulator supply bindings (Stephen Boyd)
>>  - Add ref clk optional dt binding (Stephen Boyd)
>>  - Add gpio-controller optional dt binding (Stephen Boyd)
>> 
>> Changes in v5:
>>  - Use clock property to specify the input refclk (Stephen Boyd).
>>  - Update gpio cell and pwm cell numbers (Stephen Boyd).
>> 
>> Changes in v6:
>>  - Add property to mention the lane mapping scheme and polarity 
>> inversion
>>    (Stephen Boyd).
>> 
>> Changes in v7:
>>  - Detail description of lane mapping scheme dt property (Andrzej
>>    Hajda/ Rob Herring).
>>  - Removed HDP gpio binding, since the bridge uses IRQ signal to
>>    determine HPD, and IRQ property is already documented in binding.
>> 
>> Changes in v8:
>>  - Removed unnecessary explanation of lane mapping and polarity dt
>>    property, since these are already explained in 
>> media/video-interface
>>    dt binidng (Rob Herring).
>> 
>> Changes in v9:
>>  - Avoid putting re-definition of lane mapping and polarity dt binding
>>    (Rob Herring).
>> 
>> Signed-off-by: Sandeep Panda <spanda@codeaurora.org>
>> ---
>>  .../bindings/display/bridge/ti,sn65dsi86.txt       | 87 
>> ++++++++++++++++++++++
>>  1 file changed, 87 insertions(+)
>>  create mode 100644 
>> Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt 
>> b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
>> new file mode 100644
>> index 0000000..507efbb
>> --- /dev/null
>> +++ 
>> b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
>> @@ -0,0 +1,87 @@
>> +SN65DSI86 DSI to eDP bridge chip
>> +--------------------------------
>> +
>> +This is the binding for Texas Instruments SN65DSI86 bridge.
>> +http://www.ti.com/general/docs/lit/getliterature.tsp?genericPartNumber=sn65dsi86&fileType=pdf
>> +
>> +Required properties:
>> +- compatible: Must be "ti,sn65dsi86"
>> +- reg: i2c address of the chip, 0x2d as per datasheet
>> +- enable-gpios: OF device-tree gpio specification for bridge_en pin 
>> (active high)
>> +
>> +- vccio-supply: A 1.8V supply that powers up the digital IOs.
>> +- vpll-supply: A 1.8V supply that powers up the displayport PLL.
>> +- vcca-supply: A 1.2V supply that powers up the analog circuits.
>> +- vcc-supply: A 1.2V supply that powers up the digital core.
> 
> Since there are only two voltage levels (1.8 and 1.2) and power on/off
> sequence does not require specific order I guess you could merge
> supplies with the same level, but this is just an idea, up to you.
> For example you can drop (or make optional) vpll-supply and 
> vcca-supply.
>> +

Since the bridge datasheet mentions about 4 different power supplies and 
there
was also comment from other reviewers to keep all of them so i have put 
it like this.
Also this will help in scenarios where board design has 4 different 
sources for these 4
power supplies.

>> +Optional properties:
>> +- interrupts: Specifier for the SN65DSI86 interrupt line.
> 
> or interrupts-extended
> 
>> +
>> +- ddc-i2c-bus: phandle of the I2C controller used for DDC EDID 
>> probing
>> +
>> +- gpio-controller: Marks the device has a GPIO controller.
>> +- #gpio-cells    : Should be two. The first cell is the pin number 
>> and
>> +                   the second cell is used to specify flags.
>> +                   See ../../gpio/gpio.txt for more information.
>> +- #pwm-cells : Should be one. See ../../pwm/pwm.txt for description 
>> of
>> +               the cell formats.
>> +
>> +- clock-names: should be "refclk"
>> +- clocks: Specification for input reference clock. The reference
>> +	  clock rate must be 12 MHz, 19.2 MHz, 26 MHz, 27 MHz or 38.4 MHz.
>> +
>> +- data-lanes: See ../../media/video-interface.txt
>> +- lane-polarities: See ../../media/video-interface.txt
> 
> Either you should describe which port these properties are related to,
> either put them into endpoint node. According to
> ./../media/video-interface.txt only the latter is correct.
> 

Data lane and lane-polarity is applicable to both DSI and eDP interface. 
I have
updated the same in ../../media/video-interface.txt. Do you want to 
explicitly
mention here that this property is for eDP data lanes mapping?

>> +
>> +Required nodes:
>> +This device has two video ports. Their connections are modelled using 
>> the
>> +OF graph bindings specified in 
>> Documentation/devicetree/bindings/graph.txt.
>> +
>> +- Video port 0 for DSI input
>> +- Video port 1 for eDP output
>> +
>> +Example
>> +-------
>> +
>> +edp-bridge@2d {
>> +	compatible = "ti,sn65dsi86";
>> +	#address-cells = <1>;
>> +	#size-cells = <0>;
>> +	reg = <0x2d>;
>> +
>> +	enable-gpios = <&msmgpio 33 GPIO_ACTIVE_HIGH>;
>> +	interrupt-parent = <&gpio3>;
>> +	interrupts = <4 IRQ_TYPE_EDGE_FALLING>;
> 
> In case of non-default interrupt parent interrupts-extended is more
> compact/convenient - you can drop interrupt-parent property:
>     interrupts-extended =  <&gpio3 4 IRQ_TYPE_EDGE_FALLING>;

Ok. will update in next patch.
> 
>> +
>> +	vccio-supply = <&pm8916_l17>;
>> +	vcca-supply = <&pm8916_l6>;
>> +	vpll-supply = <&pm8916_l17>;
>> +	vcc-supply = <&pm8916_l6>;
>> +
>> +	clock-names = "refclk";
>> +	clocks = <&input_refclk>;
>> +
>> +	data-lanes = <2 1 3 0>;
>> +	lane-polarities = <0 1 0 1>;
> 
> And this should be fixed as well.
Ok i will move these to edp out endpoint node.
> 
>> +
>> +	ports {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		port@0 {
>> +			reg = <0>;
>> +
>> + 			edp_bridge_in: endpoint {
>> +				remote-endpoint = <&dsi_out>;
>> +			};
>> +		};
>> +
>> +		port@1 {
>> +			reg = <1>;
>> +
>> +			edp_bridge_out: endpoint {
>> +				remote-endpoint = <&edp_panel_in>;
>> +			};
>> +		};
>> +	};
>> +}
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno