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

[Jonathan Neuschäfer]

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@xxxxxxxxxxxx>
> ---
>  .../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: signature.asc
Description: PGP signature