Re: [PATCH 1/2] drm: panel: Add Samsung s6e63m0 panel documentation

[PaweÅ Chmiel]

On sobota, 26 stycznia 2019 21:55:01 CET Sam Ravnborg wrote:
Hi
> Hi Pawel.
> 
> Thanks for the patch, some comments follows.
> Please judge what comments you chose to follow, see this as suggestions.
> 
> According to Documentation/devicetree/bindings/submitting-patches.rst:
> 
> 	The preferred subject prefix for binding patches is:
> 	"dt-bindings: <binding dir>: ..."
> 
> It would be a good idea to follow this practice in next revision.
I don't know how I forgot about this (will be fixed in next version).
> 
> On Fri, Jan 25, 2019 at 05:46:44PM +0100, PaweÅ Chmiel wrote:
> > From: Jonathan Bakker <xc-racer2@xxxxxxx>
> > 
> > This commit adds documentation for Samsung s6e63m0 AMOLED LCD panel
> > driver.
> > 
> > Signed-off-by: Jonathan Bakker <xc-racer2@xxxxxxx>
> > Signed-off-by: PaweÅ Chmiel <pawel.mikolaj.chmiel@xxxxxxxxx>
> > ---
> >  .../display/panel/samsung,s6e63m0.txt         | 60 +++++++++++++++++++
> >  1 file changed, 60 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/display/panel/samsung,s6e63m0.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/display/panel/samsung,s6e63m0.txt b/Documentation/devicetree/bindings/display/panel/samsung,s6e63m0.txt
> > new file mode 100644
> > index 000000000000..4979200e2dd2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/panel/samsung,s6e63m0.txt
> > @@ -0,0 +1,60 @@
> > +Samsung s6e63m0 AMOLED LCD panel
> > +
> > +Required properties:
> > +  - compatible: "samsung,s6e63m0"
> > +  - reset-gpio: GPIO spec for reset pin
> The preferred name is reset-gpios (added 's')
Right, will be fixed.
> 
> > +  - vdd3-supply: VDD regulator
> > +  - vci-supply: VCI regulator
> > +  - display-timings: timings for the connected panel as described by [1]
> Today, as is my best understanding, it is encouraged to specify the timing
> in the actual driver and not in DT,
Ok, will hardcode them in driver. Currently those timings (which i had added to my device dts) were taken from original kernel sources.
Need to check if there are other devices (not only using mainline kernel) using this panel and what timings are they using (hope they're the same).
> 
> The example include a spi-max-frequency which is not mentioned?
spi-max-frequency shouldn't be here and will be removed.
> 
> > +
> > +Optional properties:
> > +  - reset-delay: Delay in ms after adjusting reset-gpio, default 120ms
> > +  - power-on-delay: Delay in ms after powering on, default 25ms
> > +  - power-off-delay: Delay in ms before powering off, default 200ms
> > +  - panel-width-mm: physical panel width in mm
> > +  - panel-height-mm: physical panel height in mm
> Likewise these delays are also properties that today are included in the driver.
> 
Need to check delays also (like timings).
> I cannot explain the background for this, this is just how things are done.
> 
> > +
> > +The device node can contain one 'port' child node with one child
> > +'endpoint' node, according to the bindings defined in [2]. This
> > +node should describe panel's video bus.
> > +
> > +[1]: Documentation/devicetree/bindings/display/panel/display-timing.txt
> > +[2]: Documentation/devicetree/bindings/media/video-interfaces.txt
> > +
> > +Example:
> > +
> > +		s6e63m0: display@0 {
> > +			compatible = "samsung,s6e63m0";
> > +			reg = <0>;
> > +			reset-gpio = <&mp05 5 1>;
> > +			vdd3-supply = <&ldo12_reg>;
> > +			vci-supply = <&ldo11_reg>;
> > +			spi-max-frequency = <1000000>;
> > +
> > +			power-on-delay = <0>;
> > +			power-off-delay = <0>;
> > +			reset-delay = <10>;
> > +			panel-width-mm = <53>;
> > +			panel-height-mm = <89>;
> > +
> > +			display-timings {
> > +				timing-0 {
> > +					/* 480x800@60Hz */
> > +					clock-frequency = <25628040>;
> > +					hactive = <480>;
> > +					vactive = <800>;
> > +					hfront-porch = <16>;
> > +					hback-porch = <16>;
> > +					hsync-len = <2>;
> > +					vfront-porch = <28>;
> > +					vback-porch = <1>;
> > +					vsync-len = <2>;
> > +				};
> > +			};
> > +
> > +			port {
> > +				lcd_ep: endpoint {
> > +					remote-endpoint = <&fimd_ep>;
> > +				};
> > +			};
> > +		};
> 
> 	Sam
> 
Thanks for review