Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sat, 26 Jan 2019 21:55:01 +0100
From:   Sam Ravnborg <sam@ravnborg.org>
To:     =?utf-8?B?UGF3ZcWC?= Chmiel <pawel.mikolaj.chmiel@gmail.com>
Cc:     thierry.reding@gmail.com, mark.rutland@arm.com,
        devicetree@vger.kernel.org, airlied@linux.ie,
        Jonathan Bakker <xc-racer2@live.ca>,
        linux-kernel@vger.kernel.org, krzk@kernel.org, robh+dt@kernel.org,
        dri-devel@lists.freedesktop.org, m.szyprowski@samsung.com
Subject: Re: [PATCH 1/2] drm: panel: Add Samsung s6e63m0 panel documentation
Message-ID: <20190126205501.GA31182@ravnborg.org>
References: <20190125164645.19208-1-pawel.mikolaj.chmiel@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20190125164645.19208-1-pawel.mikolaj.chmiel@gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.

On Fri, Jan 25, 2019 at 05:46:44PM +0100, Paweł Chmiel wrote:
> From: Jonathan Bakker <xc-racer2@live.ca>
> 
> This commit adds documentation for Samsung s6e63m0 AMOLED LCD panel
> driver.
> 
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> ---
>  .../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')

> +  - 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,

The example include a spi-max-frequency which is not mentioned?

> +
> +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.

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