| Message ID | 20220206080016.796556-2-andreas@kemnade.info (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: EPDC driver for i.MX6 | expand |
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 > >
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
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
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
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
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
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
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 --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>; + }; + }; +...
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