Re: [PATCH v3 3/6] dt-bindings: Add panel-timing subnode to simple-panel

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Sean,

Thank you for the patch.

On Thursday, 8 February 2018 19:48:50 EET Sean Paul wrote:
> This patch adds a new subnode to simple-panel allowing us to override
> the typical timing expressed in the panel's display_timing.
> 
> Changes in v2:
>  - Split out the binding into a new patch (Rob)
>  - display-timings is a new section (Rob)
>  - Use the full display-timings subnode instead of picking the timing
>    out (Rob/Thierry)
> Changes in v3:
>  - Go back to using the timing subnode directly, but rename to
>    panel-timing (Rob)
> 
> Cc: Doug Anderson <dianders@chromium.org>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Cc: Jeffy Chen <jeffy.chen@rock-chips.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Stéphane Marchesin <marcheu@chromium.org>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: devicetree@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-rockchip@lists.infradead.org
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  .../bindings/display/panel/simple-panel.txt        | 30 +++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git
> a/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> b/Documentation/devicetree/bindings/display/panel/simple-panel.txt index
> 45a457ad38f0..7788b9ce160b 100644
> --- a/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> +++ b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> @@ -12,6 +12,24 @@ Optional properties:
>  - enable-gpios: GPIO pin to enable or disable the panel
>  - backlight: phandle of the backlight device attached to the panel
> 
> +panel-timing subnode
> +--------------------
> +
> +This optional subnode is for devices which require a mode differing from
> the
> +panel's "typical" display timing as programmed in the simple-panel driver.

Please don't refer to particular drivers in DT bindings.

> +Overriding the driver mode must only be done in the following scenario:
> + - The restrictions motivating the override cannot be applied to the
> platform's
> +   display driver (ie: it must be specific to the device not the platform)

I find this quite ambiguous. I assume that "not the platform" refers to the 
platform's display controller. "platform" without a qualifier is too vague, as 
I expect the reason to override the mode will come from the platform's 
specific interactions between the panel, the display controller, the other 
display-related devices if any, and the PCB.

> + - The panel must not have a fixed mode attributed to it in the driver

I'm not sure to follow you here.

> + - The panel must provide at list one display_timing range by which the
> override
> +   mode can be validated against
> + - The override mode will use the 'typ' values from the panel-timings
> subnode

The panel-timings subnode has no "typ" values. Do you mean that the 
display_timing for the panel will have its "typ" values overridden by the 
values contained in the panel-timing node ?

> + - You must provide all required properties for the panel-timing subnode
> +
> +Format information on the panel-timing subnode can be found in
> +display-timing.txt.
> +
> +
>  Example:
> 
>  	panel: panel {
> @@ -22,4 +40,16 @@ Example:
>  		enable-gpios = <&gpio 90 0>;
> 
>  		backlight = <&backlight>;
> +
> +		panel-timing {
> +			clock-frequency = <266604720>;
> +			hactive = <2400>;
> +			hfront-porch = <48>;
> +			hback-porch = <84>;
> +			hsync-len = <32>;
> +			vactive = <1600>;
> +			vfront-porch = <3>;
> +			vback-porch = <120>;
> +			vsync-len = <10>;
> +		};
>  	};

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel