Re: [PATCH v5 2/2] dt-bindings: Add Truly NT35597 panel bindings

[abhinavk@codeaurora.org]

Hi Linus

Thanks for your valuable comments.

Yes, we agree with your comments here and will address them.

Some details below on how we will take care of it.

Thanks

Abhinav

On 2018-08-03 04:20, Linus Walleij wrote:
> On Fri, Aug 3, 2018 at 4:49 AM Abhinav Kumar <abhinavk@codeaurora.org> 
> wrote:
> 
>> From: "abhinavk@codeaurora.org" <abhinavk@codeaurora.org>
>> 
>> Add the device tree bindings for Truly NT35597 panel.
>> This panel supports both single DSI and dual DSI.
> 
> I do not think this is a panel but a panel driver that can be used
> with several physical panels. Can you confirm?
Yes, from the documentation I have I can see that this is a panel
driver and can support other panels/resolutions.
> 
> I suspect this since the command sequence in the driver seems
> to contain a command for setting up the actual resolution and
> a bunch of clocking properties for the physical panel.
> 
>> +Required properties:
>> +- compatible: should be "truly,nt35597"
> 
> As with eg ili9322 I think this should have dual compatible strings
> identifying the system it is used with since the resolution, clocking
> etc is apparently
> actually configurable.
> 
> compatible:
>   "truly,nt35597", "qcom,reference-design1-name-display";
>   "truly,nt35597", "qcom,reference-design2-name-display";
> 
> Then you send the command setting up resolution etc only for that
> one system.
> 
Yes, I agree we will can have dual compatible strings and we will pick
resolution/modes based on the compatible string similar to this:

https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/panel/panel-ilitek-ili9322.c#L659

>> +- vdda-supply: phandle of the regulator that provides the supply 
>> voltage
>> +  Power IC supply
>> +- vdispp-supply: phandle of the regulator that provides the supply 
>> voltage
>> +  for positive LCD bias
>> +- vdispn-supply: phandle of the regulator that provides the supply 
>> voltage
>> +  for negative LCD bias
>> +- reset-gpios: phandle of gpio for reset line
>> +  This should be 8mA, gpio can be configured using mux, pinctrl, 
>> pinctrl-names
>> +- mode-gpios: phandle of the gpio for choosing the mode of the 
>> display
>> +  for single DSI or Dual DSI
>> +  This should be low for dual DSI and high for single DSI mode
>> +- display-timings: Node for the Panel timings
> 
> I don't think this belongs in the device tree at all.
> 
> Provide the timings from the driver based on the compatible string
> instead, as you actually send commands to set up a certain timing in
> the display controller.
> 
> (See ili9322 driver for an example of how I do this.)

Yes, will follow the example of ili9322 and do the same.

> 
>> +- ports: This device has two video ports driven by two DSIs. Their 
>> connections
>> +  are modelled using the OF graph bindings specified in
>> +  Documentation/devicetree/bindings/graph.txt.
>> +  - port@0: DSI input port driven by master DSI
>> +  - port@1: DSI input port driven by secondary DSI
> 
> The rest seems fine.
> 
> Yours,
> Linus Walleij
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel