diff mbox series

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

Message ID 20220206080016.796556-2-andreas@kemnade.info (mailing list archive)
State New, archived
Headers show
Series drm: EPDC driver for i.MX6 | expand

Commit Message

Andreas Kemnade Feb. 6, 2022, 8 a.m. UTC
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

Comments

Rob Herring Feb. 11, 2022, 3:46 p.m. UTC | #1
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 first version was in i.MX50 (I helped design the register 
interface). Is that version compatible?

> 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
> +---
> +$id: http://devicetree.org/schemas/display/imx/fsl,mxc-epdc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Freescale i.MX6 EPDC
> +
> +maintainers:
> +  - Andreas Kemnade <andreas@kemnade.info>
> +
> +description: |
> +  The EPDC is a controller for handling electronic paper displays found in
> +  i.MX6 SoCs.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - fsl,imx6sl-epdc
> +      - fsl,imx6sll-epdc

Not compatible with each other?

> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: Bus clock
> +      - description: Pixel clock
> +
> +  clock-names:
> +    items:
> +      - const: axi
> +      - const: pix
> +
> +  interrupts:
> +    maxItems: 1
> +

> +  vscan-holdoff:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    maxItems: 1
> +
> +  sdoed-width:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    maxItems: 1
> +
> +  sdoed-delay:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    maxItems: 1
> +
> +  sdoez-width:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    maxItems: 1
> +
> +  sdoez-delay:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    maxItems: 1
> +
> +  gdclk-hp-offs:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    maxItems: 1
> +
> +  gdsp-offs:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    maxItems: 1
> +
> +  gdoe-offs:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    maxItems: 1
> +
> +  gdclk-offs:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    maxItems: 1
> +
> +  num-ce:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    maxItems: 1

All these need a vendor prefix and descriptions.

> +
> +  timing:
> +    $ref: /display/panel/panel-timing.yaml#
> +
> +  DISPLAY-supply:
> +    description:
> +      A couple of +/- voltages automatically powered on in a defintive order
> +
> +  VCOM-supply:
> +    description: compensation voltage
> +
> +  V3P3-supply:
> +    description: V3P3 supply
> +
> +  epd-thermal-zone:
> +    description:
> +      Zone to get temperature of the EPD from, practically ambient temperature.
> +
> +
> +

1 blank line.

> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - interrupts
> +  - vscan-holdoff
> +  - sdoed-width
> +  - sdoed-delay
> +  - sdoez-width
> +  - sdoez-delay
> +  - gdclk-hp-offs
> +  - gdsp-offs
> +  - gdoe-offs
> +  - gdclk-offs
> +  - num-ce
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/imx6sl-clock.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    epdc: epdc@20f4000 {
> +        compatible = "fsl,imx6sl-epdc";
> +        reg = <0x020f4000 0x4000>;
> +        interrupts = <0 97 IRQ_TYPE_LEVEL_HIGH>;
> +        clocks = <&clks IMX6SL_CLK_EPDC_AXI>, <&clks IMX6SL_CLK_EPDC_PIX>;
> +        clock-names = "axi", "pix";
> +
> +        pinctrl-names = "default";
> +        pinctrl-0 = <&pinctrl_epdc0>;
> +        V3P3-supply = <&V3P3_reg>;
> +        VCOM-supply = <&VCOM_reg>;
> +        DISPLAY-supply = <&DISPLAY_reg>;
> +        epd-thermal-zone = "epd-thermal";
> +
> +        vscan-holdoff = <4>;
> +        sdoed-width = <10>;
> +        sdoed-delay = <20>;
> +        sdoez-width = <10>;
> +        sdoez-delay = <20>;
> +        gdclk-hp-offs = <562>;
> +        gdsp-offs = <662>;
> +        gdoe-offs = <0>;
> +        gdclk-offs = <225>;
> +        num-ce = <3>;
> +        status = "okay";

Don't need status in examples.

> +
> +        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>;
> +        };
> +    };
> +...
> -- 
> 2.30.2
> 
>
Andreas Kemnade Feb. 14, 2022, 10:45 p.m. UTC | #2
Hi Rob,

On Fri, 11 Feb 2022 09:46:27 -0600
Rob Herring <robh@kernel.org> wrote:

> 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 first version was in i.MX50 (I helped design the register 
> interface). Is that version compatible?
> 
it has some differences, but that could be detected by EPDC_VERSION
register. I do not own such a device, so I cannot fully check. I have
not seen any driver with devicetree for IMX5. For now I am rejecting
anything which has a EPDC version which I cannot check. 

> > 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
> > +---
> > +$id: http://devicetree.org/schemas/display/imx/fsl,mxc-epdc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Freescale i.MX6 EPDC
> > +
> > +maintainers:
> > +  - Andreas Kemnade <andreas@kemnade.info>
> > +
> > +description: |
> > +  The EPDC is a controller for handling electronic paper displays found in
> > +  i.MX6 SoCs.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - fsl,imx6sl-epdc
> > +      - fsl,imx6sll-epdc  
> 
> Not compatible with each other?
> 
differences are detectable by EPDC_VERSION register, so probably so
problem. NXP/Freescale kernel uses
fsl,imx6dl-epdc
and 
fsl,imx7d-epdc (used also by imx6 devices with EPDC_VERSION = 3.0)
in their drivers.

fsl,imx6dl-epdc
fsl,imx6sl-epdc
fsl,imx6sll-epdc
fsl,imx7d-epdc
in their dtsis.

But the general rule is to use as less as possible compatible strings
if differences can be probed properly, so only one should be
sufficient? Which one?

Regards,
Andreas
Rob Herring Feb. 16, 2022, 11:52 p.m. UTC | #3
On Mon, Feb 14, 2022 at 11:45:17PM +0100, Andreas Kemnade wrote:
> Hi Rob,
> 
> On Fri, 11 Feb 2022 09:46:27 -0600
> Rob Herring <robh@kernel.org> wrote:
> 
> > 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 first version was in i.MX50 (I helped design the register 
> > interface). Is that version compatible?
> > 
> it has some differences, but that could be detected by EPDC_VERSION
> register. I do not own such a device, so I cannot fully check. I have
> not seen any driver with devicetree for IMX5. For now I am rejecting
> anything which has a EPDC version which I cannot check. 
> 
> > > 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
> > > +---
> > > +$id: http://devicetree.org/schemas/display/imx/fsl,mxc-epdc.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Freescale i.MX6 EPDC
> > > +
> > > +maintainers:
> > > +  - Andreas Kemnade <andreas@kemnade.info>
> > > +
> > > +description: |
> > > +  The EPDC is a controller for handling electronic paper displays found in
> > > +  i.MX6 SoCs.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - fsl,imx6sl-epdc
> > > +      - fsl,imx6sll-epdc  
> > 
> > Not compatible with each other?
> > 
> differences are detectable by EPDC_VERSION register, so probably so
> problem. NXP/Freescale kernel uses
> fsl,imx6dl-epdc
> and 
> fsl,imx7d-epdc (used also by imx6 devices with EPDC_VERSION = 3.0)
> in their drivers.
> 
> fsl,imx6dl-epdc
> fsl,imx6sl-epdc
> fsl,imx6sll-epdc
> fsl,imx7d-epdc
> in their dtsis.
> 
> But the general rule is to use as less as possible compatible strings
> if differences can be probed properly, so only one should be
> sufficient? Which one?

If you can probe all the differences, then just 'fsl,imx-epdc' is 
sufficient. Just document that so the next time around I don't forget 
and tell you it needs to be specific.

Rob
Krzysztof Kozlowski Feb. 17, 2022, 9:21 a.m. UTC | #4
On 06/02/2022 09:00, 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
> +---
> +$id: http://devicetree.org/schemas/display/imx/fsl,mxc-epdc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Freescale i.MX6 EPDC
> +
> +maintainers:
> +  - Andreas Kemnade <andreas@kemnade.info>
> +
> +description: |
> +  The EPDC is a controller for handling electronic paper displays found in
> +  i.MX6 SoCs.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - fsl,imx6sl-epdc
> +      - fsl,imx6sll-epdc
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: Bus clock
> +      - description: Pixel clock
> +
> +  clock-names:
> +    items:
> +      - const: axi
> +      - const: pix
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  vscan-holdoff:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    maxItems: 1

Except what Rob already said, all these are not arrays, so maxItems is
not appropriate. You can define minimum/maximum values instead.

> +
> +  sdoed-width:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    maxItems: 1
> +
> +  sdoed-delay:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    maxItems: 1
> +
> +  sdoez-width:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    maxItems: 1
> +
> +  sdoez-delay:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    maxItems: 1
> +
> +  gdclk-hp-offs:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    maxItems: 1
> +
> +  gdsp-offs:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    maxItems: 1
> +
> +  gdoe-offs:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    maxItems: 1
> +
> +  gdclk-offs:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    maxItems: 1
> +
> +  num-ce:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    maxItems: 1
> +
> +  timing:
> +    $ref: /display/panel/panel-timing.yaml#
> +
> +  DISPLAY-supply:
> +    description:
> +      A couple of +/- voltages automatically powered on in a defintive order

Typo, definitive?

> +
> +  VCOM-supply:
> +    description: compensation voltage
> +
> +  V3P3-supply:

All of supplies names - lowercase.

> +    description: V3P3 supply
> +
> +  epd-thermal-zone:
> +    description:
> +      Zone to get temperature of the EPD from, practically ambient temperature.

Is it a phandle?

> +
> +
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - interrupts
> +  - vscan-holdoff
> +  - sdoed-width
> +  - sdoed-delay
> +  - sdoez-width
> +  - sdoez-delay
> +  - gdclk-hp-offs
> +  - gdsp-offs
> +  - gdoe-offs
> +  - gdclk-offs
> +  - num-ce
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/imx6sl-clock.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    epdc: epdc@20f4000 {

Generic node name, e.g. display-controller

> +        compatible = "fsl,imx6sl-epdc";
> +        reg = <0x020f4000 0x4000>;
> +        interrupts = <0 97 IRQ_TYPE_LEVEL_HIGH>;

s/0/GIC_SPI/

> +        clocks = <&clks IMX6SL_CLK_EPDC_AXI>, <&clks IMX6SL_CLK_EPDC_PIX>;
> +        clock-names = "axi", "pix";
> +


Best regards,
Krzysztof
Andreas Kemnade Feb. 17, 2022, 11:31 a.m. UTC | #5
On Thu, 17 Feb 2022 10:21:15 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote:

> On 06/02/2022 09:00, 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
> > 
[..]

> > +
> > +  DISPLAY-supply:
> > +    description:
> > +      A couple of +/- voltages automatically powered on in a defintive order  
> 
> Typo, definitive?
> 
yes, of course.

> > +
> > +  VCOM-supply:
> > +    description: compensation voltage
> > +
> > +  V3P3-supply:  
> 
> All of supplies names - lowercase.
> 
> > +    description: V3P3 supply
> > +
> > +  epd-thermal-zone:
> > +    description:
> > +      Zone to get temperature of the EPD from, practically ambient temperature.  
> 
> Is it a phandle?
> 
a string used in
       of_property_read_string(priv->drm.dev->of_node,
                                "epd-thermal-zone", &thermal);
        if (thermal) {
                priv->thermal = thermal_zone_get_zone_by_name(thermal);
                if (IS_ERR(priv->thermal))
                        return dev_err_probe(priv->drm.dev, PTR_ERR(priv->thermal),
                                             "unable to get thermal");
        }

[...]
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/imx6sl-clock.h>
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    epdc: epdc@20f4000 {  
> 
> Generic node name, e.g. display-controller
> 
hmm, does IHMO not make too much sense here. E.g. in the imx6sll.dtsi
we have lcd-controller next to it. So having epd-controller? But that
is exactly what epdc stands for.

Regards,
Andreas
Krzysztof Kozlowski Feb. 17, 2022, 11:43 a.m. UTC | #6
On 17/02/2022 12:31, Andreas Kemnade wrote:
> On Thu, 17 Feb 2022 10:21:15 +0100
> Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote:
> 
>> On 06/02/2022 09:00, 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
>>>
> [..]
> 
>>> +
>>> +  DISPLAY-supply:
>>> +    description:
>>> +      A couple of +/- voltages automatically powered on in a defintive order  
>>
>> Typo, definitive?
>>
> yes, of course.
> 
>>> +
>>> +  VCOM-supply:
>>> +    description: compensation voltage
>>> +
>>> +  V3P3-supply:  
>>
>> All of supplies names - lowercase.
>>
>>> +    description: V3P3 supply
>>> +
>>> +  epd-thermal-zone:
>>> +    description:
>>> +      Zone to get temperature of the EPD from, practically ambient temperature.  
>>
>> Is it a phandle?
>>
> a string used in
>        of_property_read_string(priv->drm.dev->of_node,
>                                 "epd-thermal-zone", &thermal);
>         if (thermal) {
>                 priv->thermal = thermal_zone_get_zone_by_name(thermal);
>                 if (IS_ERR(priv->thermal))
>                         return dev_err_probe(priv->drm.dev, PTR_ERR(priv->thermal),
>                                              "unable to get thermal");
>         }

OK, then:
$ref: /schemas/types.yaml#/definitions/string

> 
> [...]
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/clock/imx6sl-clock.h>
>>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>>> +
>>> +    epdc: epdc@20f4000 {  
>>
>> Generic node name, e.g. display-controller
>>
> hmm, does IHMO not make too much sense here. E.g. in the imx6sll.dtsi
> we have lcd-controller next to it. So having epd-controller? But that
> is exactly what epdc stands for.

Still we have "lcd-controller", not "lcdc". Since this is only for epd,
then "epd-controller" seems reasonable. The same as we use
"interrupt-controller" (not "ic"), "dma-controller" (not "dmac" or "dc")
and so on. See also list of recommended generic names from DT specification.

Best regards,
Krzysztof
J. Neuschäfer March 12, 2022, 7:23 p.m. UTC | #7
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
Andreas Kemnade March 14, 2022, 10:04 p.m. UTC | #8
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
diff mbox series

Patch

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
+---
+$id: http://devicetree.org/schemas/display/imx/fsl,mxc-epdc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Freescale i.MX6 EPDC
+
+maintainers:
+  - Andreas Kemnade <andreas@kemnade.info>
+
+description: |
+  The EPDC is a controller for handling electronic paper displays found in
+  i.MX6 SoCs.
+
+properties:
+  compatible:
+    enum:
+      - fsl,imx6sl-epdc
+      - fsl,imx6sll-epdc
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: Bus clock
+      - description: Pixel clock
+
+  clock-names:
+    items:
+      - const: axi
+      - const: pix
+
+  interrupts:
+    maxItems: 1
+
+  vscan-holdoff:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    maxItems: 1
+
+  sdoed-width:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    maxItems: 1
+
+  sdoed-delay:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    maxItems: 1
+
+  sdoez-width:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    maxItems: 1
+
+  sdoez-delay:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    maxItems: 1
+
+  gdclk-hp-offs:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    maxItems: 1
+
+  gdsp-offs:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    maxItems: 1
+
+  gdoe-offs:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    maxItems: 1
+
+  gdclk-offs:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    maxItems: 1
+
+  num-ce:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    maxItems: 1
+
+  timing:
+    $ref: /display/panel/panel-timing.yaml#
+
+  DISPLAY-supply:
+    description:
+      A couple of +/- voltages automatically powered on in a defintive order
+
+  VCOM-supply:
+    description: compensation voltage
+
+  V3P3-supply:
+    description: V3P3 supply
+
+  epd-thermal-zone:
+    description:
+      Zone to get temperature of the EPD from, practically ambient temperature.
+
+
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - interrupts
+  - vscan-holdoff
+  - sdoed-width
+  - sdoed-delay
+  - sdoez-width
+  - sdoez-delay
+  - gdclk-hp-offs
+  - gdsp-offs
+  - gdoe-offs
+  - gdclk-offs
+  - num-ce
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/imx6sl-clock.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    epdc: epdc@20f4000 {
+        compatible = "fsl,imx6sl-epdc";
+        reg = <0x020f4000 0x4000>;
+        interrupts = <0 97 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&clks IMX6SL_CLK_EPDC_AXI>, <&clks IMX6SL_CLK_EPDC_PIX>;
+        clock-names = "axi", "pix";
+
+        pinctrl-names = "default";
+        pinctrl-0 = <&pinctrl_epdc0>;
+        V3P3-supply = <&V3P3_reg>;
+        VCOM-supply = <&VCOM_reg>;
+        DISPLAY-supply = <&DISPLAY_reg>;
+        epd-thermal-zone = "epd-thermal";
+
+        vscan-holdoff = <4>;
+        sdoed-width = <10>;
+        sdoed-delay = <20>;
+        sdoez-width = <10>;
+        sdoez-delay = <20>;
+        gdclk-hp-offs = <562>;
+        gdsp-offs = <662>;
+        gdoe-offs = <0>;
+        gdclk-offs = <225>;
+        num-ce = <3>;
+        status = "okay";
+
+        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>;
+        };
+    };
+...