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

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

On Sat, 12 Mar 2022 20:23:48 +0100
Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote:

> 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.
> 
First of all I think I should document them better (as said in an
earlier review mail)

I doubt there are sane defaults, in vendor kernels, there is typically a
definition of these parameters and a video mode per display.

> 
> > +
> > +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).
> 
IMHO it makes sense to store these timing parameters together with the
timing parameters from above. If that all somehow comes from a panel
driver, we need to design an interface for it. So for simplicity I
added the stuff just to the EPDC node.

Vendor kernel has this:
struct imx_epdc_fb_mode {
    struct fb_videomode *vmode;
    int vscan_holdoff;
    int sdoed_width;
    int sdoed_delay;
    int sdoez_width;
    int sdoez_delay;
    int gdclk_hp_offs;
    int gdsp_offs;
    int gdoe_offs;
    int gdclk_offs;
    int num_ce;
};

So things are basically combined here.

Regards,
Andreas