Re: [RFC PATCH 1/6] dt-bindings: display: imx: Add EPDC

Jonathan Neuschäfer
2022-03-12T19:24:43Z

[-- Attachment #1: Type: text/plain, Size: 2770 bytes --]

Hello Andreas,

Sorry for the delay, I finally got around to having a look at the
patchset.

Some comments from skimming the patches below, and in my other replies.

On Sun, Feb 06, 2022 at 09:00:11AM +0100, Andreas Kemnade wrote:
> Add a binding for the Electrophoretic Display Controller found at least
> in the i.MX6.
> The timing subnode is directly here to avoid having display parameters
> spread all over the plate.
>
> Supplies are organized the same way as in the fbdev driver in the
> NXP/Freescale kernel forks. The regulators used for that purpose,
> like the TPS65185, the SY7636A and MAX17135 have typically a single bit to
> start a bunch of regulators of higher or negative voltage with a
> well-defined timing. VCOM can be handled separately, but can also be
> incorporated into that single bit.
>
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
> .../bindings/display/imx/fsl,mxc-epdc.yaml | 159 ++++++++++++++++++
> 1 file changed, 159 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/display/imx/fsl,mxc-epdc.yaml
>
> diff --git a/Documentation/devicetree/bindings/display/imx/fsl,mxc-epdc.yaml b/Documentation/devicetree/bindings/display/imx/fsl,mxc-epdc.yaml
> new file mode 100644
> index 000000000000..7e0795cc3f70
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/imx/fsl,mxc-epdc.yaml
> @@ -0,0 +1,159 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
[...]
> + - vscan-holdoff
> + - sdoed-width
> + - sdoed-delay
> + - sdoez-width
> + - sdoez-delay
> + - gdclk-hp-offs
> + - gdsp-offs
> + - gdoe-offs
> + - gdclk-offs
> + - num-ce

These parameters should perhaps have sane defaults in the driver, and be
optional in the DT.

> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/imx6sl-clock.h>
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> + epdc: epdc@20f4000 {
[...]
> +
> + timing {
> + clock-frequency = <80000000>;
> + hactive = <1448>;
> + hback-porch = <16>;
> + hfront-porch = <102>;
> + hsync-len = <28>;
> + vactive = <1072>;
> + vback-porch = <4>;
> + vfront-porch = <4>;
> + vsync-len = <2>;
> + };
> + };

The way you did it here, the timing parameters are directly under the
EPDC node in the DT, but I wonder if it would be better to have a
separate node for the display panel, which can then provide the timing
parameters either in the DT or in the panel driver (selected by compatible
string of the panel).

Jonathan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]