Hi Laurent.

Good to see one of the imx bindings migrating to yaml.

On Thu, Aug 13, 2020 at 04:29:03AM +0300, Laurent Pinchart wrote:

Convert the mxsfb binding to YAML. The deprecated binding is dropped, as neither the DT sources nor the driver support it anymore.

The compatible strings are messy, and DT sources use different kinds of combination of documented and undocumented values. Keep it simple for now, and update the example to make it valid. Aligning the binding with the existing DT sources will be performed separately.

Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

Reviewed-by: Sam Ravnborg sam@ravnborg.org But see below for a few nits

---

.../devicetree/bindings/display/mxsfb.txt | 87 ------------- .../devicetree/bindings/display/mxsfb.yaml | 115 ++++++++++++++++++ MAINTAINERS | 2 +- 3 files changed, 116 insertions(+), 88 deletions(-) delete mode 100644 Documentation/devicetree/bindings/display/mxsfb.txt create mode 100644 Documentation/devicetree/bindings/display/mxsfb.yaml

diff --git a/Documentation/devicetree/bindings/display/mxsfb.txt b/Documentation/devicetree/bindings/display/mxsfb.txt deleted file mode 100644 index c985871c46b3..000000000000 --- a/Documentation/devicetree/bindings/display/mxsfb.txt +++ /dev/null @@ -1,87 +0,0 @@ -* Freescale MXS LCD Interface (LCDIF)

-New bindings:

-Required properties: -- compatible: Should be "fsl,imx23-lcdif" for i.MX23.

• Should be "fsl,imx28-lcdif" for i.MX28.
  

• Should be "fsl,imx6sx-lcdif" for i.MX6SX.
  

• Should be "fsl,imx8mq-lcdif" for i.MX8MQ.
  

-- reg: Address and length of the register set for LCDIF -- interrupts: Should contain LCDIF interrupt -- clocks: A list of phandle + clock-specifier pairs, one for each

• entry in 'clock-names'.
  

-- clock-names: A list of clock names. For MXSFB it should contain:

• • "pix" for the LCDIF block clock
• • (MX6SX-only) "axi", "disp_axi" for the bus interface clock

-Required sub-nodes:

• • port: The connection to an encoder chip.

-Example:

• lcdif1: display-controller@2220000 {

• compatible = "fsl,imx6sx-lcdif", "fsl,imx28-lcdif";
  

• reg = <0x02220000 0x4000>;
  

• interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
  

• clocks = <&clks IMX6SX_CLK_LCDIF1_PIX>,
  

• 	 <&clks IMX6SX_CLK_LCDIF_APB>,
  

• 	 <&clks IMX6SX_CLK_DISPLAY_AXI>;
  

• clock-names = "pix", "axi", "disp_axi";
  

• port {
  

• 	parallel_out: endpoint {
  

• 		remote-endpoint = <&panel_in_parallel>;
  

• 	};
  

• };
  

• };

-Deprecated bindings:

-Required properties: -- compatible: Should be "fsl,imx23-lcdif" for i.MX23.

• Should be "fsl,imx28-lcdif" for i.MX28.
  

-- reg: Address and length of the register set for LCDIF -- interrupts: Should contain LCDIF interrupts -- display: phandle to display node (see below for details)

-* display node

-Required properties: -- bits-per-pixel: <16> for RGB565, <32> for RGB888/666. -- bus-width: number of data lines. Could be <8>, <16>, <18> or <24>.

-Required sub-node: -- display-timings: Refer to binding doc display-timing.txt for details.

-Examples:

-lcdif@80030000 {

• compatible = "fsl,imx28-lcdif";
• reg = <0x80030000 2000>;
• interrupts = <38 86>;
• display: display {

• bits-per-pixel = <32>;
  

• bus-width = <24>;
  

• display-timings {
  

• 	native-mode = <&timing0>;
  

• 	timing0: timing0 {
  

• 		clock-frequency = <33500000>;
  

• 		hactive = <800>;
  

• 		vactive = <480>;
  

• 		hfront-porch = <164>;
  

• 		hback-porch = <89>;
  

• 		hsync-len = <10>;
  

• 		vback-porch = <23>;
  

• 		vfront-porch = <10>;
  

• 		vsync-len = <10>;
  

• 		hsync-active = <0>;
  

• 		vsync-active = <0>;
  

• 		de-active = <1>;
  

• 		pixelclk-active = <0>;
  

• 	};
  

• };
  

• };

-}; diff --git a/Documentation/devicetree/bindings/display/mxsfb.yaml b/Documentation/devicetree/bindings/display/mxsfb.yaml new file mode 100644 index 000000000000..202381ec5bb7 --- /dev/null +++ b/Documentation/devicetree/bindings/display/mxsfb.yaml @@ -0,0 +1,115 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/mxsfb.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml#

+title: Freescale/NXP i.MX LCD Interface (LCDIF)

+maintainers:

• • Marek Vasut marex@denx.de
• • Stefan Agner stefan@agner.ch

+description: |

• (e)LCDIF display controller found in the Freescale/NXP i.MX SoCs.

+properties:

• compatible:
• enum:

•  - fsl,imx23-lcdif
  

•  - fsl,imx28-lcdif
  

•  - fsl,imx6sx-lcdif
  

•  - fsl,imx8mq-lcdif
  

• reg:
• maxItems: 1
• clocks:
• items:

•  - description: Pixel clock
  

•  - description: Bus clock
  

•  - description: Display AXI clock
  

• minItems: 1
• clock-names:
• items:

•  - const: "pix"
  

•  - const: "axi"
  

•  - const: "disp_axi"
  

• minItems: 1
• interrupts:
• maxItems: 1
• port:
• description: The LCDIF output port
• type: object
• properties:

•  endpoint:
  

•    type: object
  

•    properties:
  

•      remote-endpoint:
  

•        $ref: /schemas/types.yaml#/definitions/phandle
  

•    required:
  

•      - remote-endpoint
  

•    additionalProperties: false
  

• additionalProperties: false

+required:

• • compatible
• • reg
• • clocks
• • interrupts
• • port

+additionalProperties: false

+allOf:

• • if:

•  properties:
  

•    compatible:
  

•      contains:
  

•        const: fsl,imx6sx-lcdif
  

• then:

•  properties:
  

•    clocks:
  

•      minItems: 2
  

•      maxItems: 3
  

•    clock-names:
  

•      minItems: 2
  

•      maxItems: 3
  

•  required:
  

•    - clock-names
  

• else:

•  properties:
  

•    clocks:
  

•      minItems: 1
  

•    clock-names:
  

•      minItems: 1
  

The else parts looks like it is not needed. The clock, clock-names properties has minItems: 1 already from above.

+examples:

• • |
• #include <dt-bindings/clock/imx6sx-clock.h>
• #include <dt-bindings/interrupt-controller/arm-gic.h>
• display-controller@2220000 {

•    compatible = "fsl,imx6sx-lcdif";
  

•    reg = <0x02220000 0x4000>;
  

•    interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
  

•    clocks = <&clks IMX6SX_CLK_LCDIF1_PIX>,
  

•             <&clks IMX6SX_CLK_LCDIF_APB>,
  

•             <&clks IMX6SX_CLK_DISPLAY_AXI>;
  

•    clock-names = "pix", "axi", "disp_axi";
  

•    port {
  

•        endpoint {
  

•            remote-endpoint = <&panel_in>;
  

•        };
  

•    };
  

• };

empty line before "..." - at least most files have it

+... diff --git a/MAINTAINERS b/MAINTAINERS index e3467e88714f..e3fac23383d2 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11757,7 +11757,7 @@ M: Stefan Agner stefan@agner.ch L: dri-devel@lists.freedesktop.org S: Supported T: git git://anongit.freedesktop.org/drm/drm-misc -F: Documentation/devicetree/bindings/display/mxsfb.txt +F: Documentation/devicetree/bindings/display/mxsfb.yaml F: drivers/gpu/drm/mxsfb/

MYLEX DAC960 PCI RAID Controller

Regards,

Laurent Pinchart

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Thu, Aug 13, 2020 at 04:29:03AM +0300, Laurent Pinchart wrote:

Convert the mxsfb binding to YAML. The deprecated binding is dropped, as neither the DT sources nor the driver support it anymore.

Ah, the first display controller I worked on...

The compatible strings are messy, and DT sources use different kinds of combination of documented and undocumented values. Keep it simple for now, and update the example to make it valid. Aligning the binding with the existing DT sources will be performed separately.

Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

.../devicetree/bindings/display/mxsfb.txt | 87 ------------- .../devicetree/bindings/display/mxsfb.yaml | 115 ++++++++++++++++++ MAINTAINERS | 2 +- 3 files changed, 116 insertions(+), 88 deletions(-) delete mode 100644 Documentation/devicetree/bindings/display/mxsfb.txt create mode 100644 Documentation/devicetree/bindings/display/mxsfb.yaml

diff --git a/Documentation/devicetree/bindings/display/mxsfb.txt b/Documentation/devicetree/bindings/display/mxsfb.txt deleted file mode 100644 index c985871c46b3..000000000000 --- a/Documentation/devicetree/bindings/display/mxsfb.txt +++ /dev/null @@ -1,87 +0,0 @@ -* Freescale MXS LCD Interface (LCDIF)

-New bindings:

-Required properties: -- compatible: Should be "fsl,imx23-lcdif" for i.MX23.

• Should be "fsl,imx28-lcdif" for i.MX28.
  

• Should be "fsl,imx6sx-lcdif" for i.MX6SX.
  

• Should be "fsl,imx8mq-lcdif" for i.MX8MQ.
  

-- reg: Address and length of the register set for LCDIF -- interrupts: Should contain LCDIF interrupt -- clocks: A list of phandle + clock-specifier pairs, one for each

• entry in 'clock-names'.
  

-- clock-names: A list of clock names. For MXSFB it should contain:

• • "pix" for the LCDIF block clock
• • (MX6SX-only) "axi", "disp_axi" for the bus interface clock

-Required sub-nodes:

• • port: The connection to an encoder chip.

-Example:

• lcdif1: display-controller@2220000 {

• compatible = "fsl,imx6sx-lcdif", "fsl,imx28-lcdif";
  

• reg = <0x02220000 0x4000>;
  

• interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
  

• clocks = <&clks IMX6SX_CLK_LCDIF1_PIX>,
  

• 	 <&clks IMX6SX_CLK_LCDIF_APB>,
  

• 	 <&clks IMX6SX_CLK_DISPLAY_AXI>;
  

• clock-names = "pix", "axi", "disp_axi";
  

• port {
  

• 	parallel_out: endpoint {
  

• 		remote-endpoint = <&panel_in_parallel>;
  

• 	};
  

• };
  

• };

-Deprecated bindings:

-Required properties: -- compatible: Should be "fsl,imx23-lcdif" for i.MX23.

• Should be "fsl,imx28-lcdif" for i.MX28.
  

-- reg: Address and length of the register set for LCDIF -- interrupts: Should contain LCDIF interrupts -- display: phandle to display node (see below for details)

-* display node

-Required properties: -- bits-per-pixel: <16> for RGB565, <32> for RGB888/666. -- bus-width: number of data lines. Could be <8>, <16>, <18> or <24>.

-Required sub-node: -- display-timings: Refer to binding doc display-timing.txt for details.

-Examples:

-lcdif@80030000 {

• compatible = "fsl,imx28-lcdif";
• reg = <0x80030000 2000>;
• interrupts = <38 86>;
• display: display {

• bits-per-pixel = <32>;
  

• bus-width = <24>;
  

• display-timings {
  

• 	native-mode = <&timing0>;
  

• 	timing0: timing0 {
  

• 		clock-frequency = <33500000>;
  

• 		hactive = <800>;
  

• 		vactive = <480>;
  

• 		hfront-porch = <164>;
  

• 		hback-porch = <89>;
  

• 		hsync-len = <10>;
  

• 		vback-porch = <23>;
  

• 		vfront-porch = <10>;
  

• 		vsync-len = <10>;
  

• 		hsync-active = <0>;
  

• 		vsync-active = <0>;
  

• 		de-active = <1>;
  

• 		pixelclk-active = <0>;
  

• 	};
  

• };
  

• };

-}; diff --git a/Documentation/devicetree/bindings/display/mxsfb.yaml b/Documentation/devicetree/bindings/display/mxsfb.yaml new file mode 100644 index 000000000000..202381ec5bb7 --- /dev/null +++ b/Documentation/devicetree/bindings/display/mxsfb.yaml @@ -0,0 +1,115 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/mxsfb.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml#

+title: Freescale/NXP i.MX LCD Interface (LCDIF)

+maintainers:

• • Marek Vasut marex@denx.de
• • Stefan Agner stefan@agner.ch

+description: |

• (e)LCDIF display controller found in the Freescale/NXP i.MX SoCs.

+properties:

• compatible:
• enum:

•  - fsl,imx23-lcdif
  

•  - fsl,imx28-lcdif
  

•  - fsl,imx6sx-lcdif
  

•  - fsl,imx8mq-lcdif
  

• reg:
• maxItems: 1
• clocks:
• items:

•  - description: Pixel clock
  

•  - description: Bus clock
  

•  - description: Display AXI clock
  

• minItems: 1
• clock-names:
• items:

•  - const: "pix"
  

•  - const: "axi"
  

•  - const: "disp_axi"
  

Don't need quotes here.

• minItems: 1
• interrupts:
• maxItems: 1
• port:
• description: The LCDIF output port
• type: object
• properties:

•  endpoint:
  

•    type: object
  

•    properties:
  

•      remote-endpoint:
  

•        $ref: /schemas/types.yaml#/definitions/phandle
  

•    required:
  

•      - remote-endpoint
  

•    additionalProperties: false
  

• additionalProperties: false

+required:

• • compatible
• • reg
• • clocks
• • interrupts
• • port

+additionalProperties: false

+allOf:

• • if:

•  properties:
  

•    compatible:
  

•      contains:
  

•        const: fsl,imx6sx-lcdif
  

• then:

•  properties:
  

•    clocks:
  

•      minItems: 2
  

•      maxItems: 3
  

•    clock-names:
  

•      minItems: 2
  

•      maxItems: 3
  

•  required:
  

•    - clock-names
  

• else:

•  properties:
  

•    clocks:
  

•      minItems: 1
  

•    clock-names:
  

•      minItems: 1
  

+examples:

• • |
• #include <dt-bindings/clock/imx6sx-clock.h>
• #include <dt-bindings/interrupt-controller/arm-gic.h>
• display-controller@2220000 {

•    compatible = "fsl,imx6sx-lcdif";
  

•    reg = <0x02220000 0x4000>;
  

•    interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
  

•    clocks = <&clks IMX6SX_CLK_LCDIF1_PIX>,
  

•             <&clks IMX6SX_CLK_LCDIF_APB>,
  

•             <&clks IMX6SX_CLK_DISPLAY_AXI>;
  

•    clock-names = "pix", "axi", "disp_axi";
  

•    port {
  

•        endpoint {
  

•            remote-endpoint = <&panel_in>;
  

•        };
  

•    };
  

• };

+... diff --git a/MAINTAINERS b/MAINTAINERS index e3467e88714f..e3fac23383d2 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11757,7 +11757,7 @@ M: Stefan Agner stefan@agner.ch L: dri-devel@lists.freedesktop.org S: Supported T: git git://anongit.freedesktop.org/drm/drm-misc -F: Documentation/devicetree/bindings/display/mxsfb.txt +F: Documentation/devicetree/bindings/display/mxsfb.yaml F: drivers/gpu/drm/mxsfb/

MYLEX DAC960 PCI RAID Controller

Regards,

Laurent Pinchart

On Thu, Aug 13, 2020 at 04:29:04AM +0300, Laurent Pinchart wrote:

Additional compatible strings have been added in DT source for the i.MX6SL, i.MX6SLL, i.MX6UL and i.MX7D without updating the bindings. Most of the upstream DT sources use the fsl,imx28-lcdif compatible string, which mostly predates the realization that the LCDIF in the i.MX6 and newer SoCs have extra features compared to the i.MX28.

Update the bindings to add the missing compatible strings, with the correct fallback values. This fails to validate some of the upstream DT sources. Instead of adding the incorrect compatible fallback to the binding, the sources should be updated separately.

Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

.../devicetree/bindings/display/mxsfb.yaml | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/mxsfb.yaml b/Documentation/devicetree/bindings/display/mxsfb.yaml index 202381ec5bb7..ec6533b1d4a3 100644 --- a/Documentation/devicetree/bindings/display/mxsfb.yaml +++ b/Documentation/devicetree/bindings/display/mxsfb.yaml @@ -15,11 +15,19 @@ description: |

properties: compatible:

• enum:

•  - fsl,imx23-lcdif
  

•  - fsl,imx28-lcdif
  

•  - fsl,imx6sx-lcdif
  

•  - fsl,imx8mq-lcdif
  

• oneOf:

•  - enum:
  

•      - fsl,imx23-lcdif
  

•      - fsl,imx28-lcdif
  

•      - fsl,imx6sx-lcdif
  

Indent correct.

•  - items:
  

•    - enum:
  

•      - fsl,imx6sl-lcdif
  

•      - fsl,imx6sll-lcdif
  

•      - fsl,imx6ul-lcdif
  

•      - fsl,imx7d-lcdif
  

•      - fsl,imx8mq-lcdif
  

Indent shall be two more spaces like above. (This is at least my best uderstanding)

•    - const: fsl,imx6sx-lcdif
  

With the above the following compatibles are supported:

"fsl,imx23-lcdif" "fsl,imx28-lcdif" "fsl,imx6sx-lcdif" "fsl,imx8mq-lcdif"

"fsl,imx6sl-lcdif", "fsl,imx6sx-lcdif" "fsl,imx6sll-lcdif", "fsl,imx6sx-lcdif" "fsl,imx6ul-lcdif", "fsl,imx6sx-lcdif" "fsl,imx7d-lcdif", "fsl,imx6sx-lcdif" "fsl,imx8mq-lcdif", "fsl,imx6sx-lcdif"

So the fallback value is the later "fsl,imx6sx-lcdif" which looks ok.

With indent fixed (or explained why I am wrong): Reviewed-by: Sam Ravnborg sam@ravnborg.org

reg: maxItems: 1 -- Regards,

Laurent Pinchart

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Mon, Aug 17, 2020 at 03:04:06AM +0300, Laurent Pinchart wrote:

Hi Sam,

On Sun, Aug 16, 2020 at 08:39:33AM +0200, Sam Ravnborg wrote:

...

On Thu, Aug 13, 2020 at 04:29:04AM +0300, Laurent Pinchart wrote:

...

Additional compatible strings have been added in DT source for the i.MX6SL, i.MX6SLL, i.MX6UL and i.MX7D without updating the bindings. Most of the upstream DT sources use the fsl,imx28-lcdif compatible string, which mostly predates the realization that the LCDIF in the i.MX6 and newer SoCs have extra features compared to the i.MX28.

Update the bindings to add the missing compatible strings, with the correct fallback values. This fails to validate some of the upstream DT sources. Instead of adding the incorrect compatible fallback to the binding, the sources should be updated separately.

Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

.../devicetree/bindings/display/mxsfb.yaml | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/mxsfb.yaml b/Documentation/devicetree/bindings/display/mxsfb.yaml index 202381ec5bb7..ec6533b1d4a3 100644 --- a/Documentation/devicetree/bindings/display/mxsfb.yaml +++ b/Documentation/devicetree/bindings/display/mxsfb.yaml @@ -15,11 +15,19 @@ description: |

properties: compatible:

• enum:

•  - fsl,imx23-lcdif
  

•  - fsl,imx28-lcdif
  

•  - fsl,imx6sx-lcdif
  

•  - fsl,imx8mq-lcdif
  

• oneOf:

•  - enum:
  

•      - fsl,imx23-lcdif
  

•      - fsl,imx28-lcdif
  

•      - fsl,imx6sx-lcdif
  

Indent correct.

...

•  - items:
  

•    - enum:
  

•      - fsl,imx6sl-lcdif
  

•      - fsl,imx6sll-lcdif
  

•      - fsl,imx6ul-lcdif
  

•      - fsl,imx7d-lcdif
  

•      - fsl,imx8mq-lcdif
  

Indent shall be two more spaces like above. (This is at least my best uderstanding)

I think you're right. I wonder why dt_binding_check doesn't complain. I'll fix it anyway.

Because I haven't integrated yamllint yet. I do have a config file at least.

...

...

•    - const: fsl,imx6sx-lcdif
  

With the above the following compatibles are supported:

"fsl,imx23-lcdif" "fsl,imx28-lcdif" "fsl,imx6sx-lcdif" "fsl,imx8mq-lcdif"

"fsl,imx6sl-lcdif", "fsl,imx6sx-lcdif" "fsl,imx6sll-lcdif", "fsl,imx6sx-lcdif" "fsl,imx6ul-lcdif", "fsl,imx6sx-lcdif" "fsl,imx7d-lcdif", "fsl,imx6sx-lcdif" "fsl,imx8mq-lcdif", "fsl,imx6sx-lcdif"

So the fallback value is the later "fsl,imx6sx-lcdif" which looks ok.

With indent fixed (or explained why I am wrong): Reviewed-by: Sam Ravnborg sam@ravnborg.org

...

reg: maxItems: 1

-- Regards,

Laurent Pinchart

Hi Stefan,

On Fri, Aug 21, 2020 at 04:53:56PM +0200, Stefan Agner wrote:

On 2020-08-13 03:29, Laurent Pinchart wrote:

...

Additional compatible strings have been added in DT source for the i.MX6SL, i.MX6SLL, i.MX6UL and i.MX7D without updating the bindings. Most of the upstream DT sources use the fsl,imx28-lcdif compatible string, which mostly predates the realization that the LCDIF in the i.MX6 and newer SoCs have extra features compared to the i.MX28.

Agreed, we should add fsl,imx6sx-lcdif for those devices.

But shouldn't we also keep fsl,imx28-lcdif? From what I can tell, the devices can be driven by a driver only supporting fsl,imx28-lcdif semantics, right?

Isn't it kept by this patch ?

...

Update the bindings to add the missing compatible strings, with the correct fallback values. This fails to validate some of the upstream DT sources. Instead of adding the incorrect compatible fallback to the binding, the sources should be updated separately.

Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

.../devicetree/bindings/display/mxsfb.yaml | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/mxsfb.yaml b/Documentation/devicetree/bindings/display/mxsfb.yaml index 202381ec5bb7..ec6533b1d4a3 100644 --- a/Documentation/devicetree/bindings/display/mxsfb.yaml +++ b/Documentation/devicetree/bindings/display/mxsfb.yaml @@ -15,11 +15,19 @@ description: |

properties: compatible:

• enum:

•  - fsl,imx23-lcdif
  

•  - fsl,imx28-lcdif
  

•  - fsl,imx6sx-lcdif
  

•  - fsl,imx8mq-lcdif
  

• oneOf:

•  - enum:
  

•      - fsl,imx23-lcdif
  

•      - fsl,imx28-lcdif
  

Here -----------------^

The binding now support any of "fsl,imx23-lcdif", "fsl,imx28-lcdif" or "fsl,imx6sx-lcdif" alone, or "fsl,imx6sx-lcdif" with another device-specific compatible string. The driver supports the three base compatible strings, for V3, V4 and V6 of the IP core.

...

•      - fsl,imx6sx-lcdif
  

•  - items:
  

•    - enum:
  

•      - fsl,imx6sl-lcdif
  

•      - fsl,imx6sll-lcdif
  

•      - fsl,imx6ul-lcdif
  

•      - fsl,imx7d-lcdif
  

•      - fsl,imx8mq-lcdif
  

•    - const: fsl,imx6sx-lcdif
  
  

  reg: maxItems: 1

Hi Stefan,

On Mon, Aug 24, 2020 at 04:19:23PM +0200, Stefan Agner wrote:

On 2020-08-24 01:26, Laurent Pinchart wrote:

...

On Fri, Aug 21, 2020 at 04:53:56PM +0200, Stefan Agner wrote:

...

On 2020-08-13 03:29, Laurent Pinchart wrote:

...

Additional compatible strings have been added in DT source for the i.MX6SL, i.MX6SLL, i.MX6UL and i.MX7D without updating the bindings. Most of the upstream DT sources use the fsl,imx28-lcdif compatible string, which mostly predates the realization that the LCDIF in the i.MX6 and newer SoCs have extra features compared to the i.MX28.

Agreed, we should add fsl,imx6sx-lcdif for those devices.

But shouldn't we also keep fsl,imx28-lcdif? From what I can tell, the devices can be driven by a driver only supporting fsl,imx28-lcdif semantics, right?

Isn't it kept by this patch ?

...

...

Update the bindings to add the missing compatible strings, with the correct fallback values. This fails to validate some of the upstream DT sources. Instead of adding the incorrect compatible fallback to the binding, the sources should be updated separately.

Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

.../devicetree/bindings/display/mxsfb.yaml | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/mxsfb.yaml b/Documentation/devicetree/bindings/display/mxsfb.yaml index 202381ec5bb7..ec6533b1d4a3 100644 --- a/Documentation/devicetree/bindings/display/mxsfb.yaml +++ b/Documentation/devicetree/bindings/display/mxsfb.yaml @@ -15,11 +15,19 @@ description: |

properties: compatible:

• enum:

•  - fsl,imx23-lcdif
  

•  - fsl,imx28-lcdif
  

•  - fsl,imx6sx-lcdif
  

•  - fsl,imx8mq-lcdif
  

• oneOf:

•  - enum:
  

•      - fsl,imx23-lcdif
  

•      - fsl,imx28-lcdif
  

Here -----------------^

The binding now support any of "fsl,imx23-lcdif", "fsl,imx28-lcdif" or "fsl,imx6sx-lcdif" alone, or "fsl,imx6sx-lcdif" with another device-specific compatible string. The driver supports the three base compatible strings, for V3, V4 and V6 of the IP core.

The binding yes, but I mean the device descriptions in the device tree.

Since the device can be driven by a older kernel which only knows about the fsl,imx28-lcdif compatible string, we could keep that compatible.

I don't think we need to care about forward-compatibility. If one updates the device tree, it's expected that the kernel should be updated accordingly. The bindings should in my opinion document the current recommended device tree properties, drivers have to ensure backward compatibility with older DT, but the other way around shouldn't be required.

From what I can tell, we can add both safely, e.g.

compatible = "fsl,imx6sx-lcdif", "fsl,imx28-lcdif"

From how I read the description this now replaces "fsl,imx28-lcdif" with "fsl,imx6sx-lcdif" for the devices supporting the additional features, e.g.:

--- a/arch/arm/boot/dts/imx6sl.dtsi +++ b/arch/arm/boot/dts/imx6sl.dtsi @@ -769,7 +769,7 @@ epdc: epdc@20f4000 { };

         lcdif: lcdif@20f8000 {

•            compatible = "fsl,imx6sl-lcdif", "fsl,imx28-lcdif";
  

•            compatible = "fsl,imx6sl-lcdif", "fsl,imx6sx-lcdif";
             reg = <0x020f8000 0x4000>;
             interrupts = <0 39 IRQ_TYPE_LEVEL_HIGH>;
             clocks = <&clks IMX6SL_CLK_LCDIF_PIX>,
  
  

...

...

...

•      - fsl,imx6sx-lcdif
  

•  - items:
  

•    - enum:
  

•      - fsl,imx6sl-lcdif
  

•      - fsl,imx6sll-lcdif
  

•      - fsl,imx6ul-lcdif
  

•      - fsl,imx7d-lcdif
  

•      - fsl,imx8mq-lcdif
  

•    - const: fsl,imx6sx-lcdif
  
  

  reg: maxItems: 1

Hi Laurent, On Thu, Aug 13, 2020 at 04:29:05AM +0300, Laurent Pinchart wrote:

When the PCB routes the display data signals in an unconventional way, the output bus width may differ from the bus width of the connected panel or encoder. For instance, when a 18-bit RGB panel has its R[5:0], G[5:0] and B[5:0] signals connected to LCD_DATA[7:2], LCD_DATA[15:10] and LCD_DATA[23:18], the output bus width is 24 instead of 18 when the signals are routed to LCD_DATA[5:0], LCD_DATA[11:6] and LCD_DATA[17:12].

Add a bus-width property to describe this data routing.

Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

Documentation/devicetree/bindings/display/mxsfb.yaml | 12 ++++++++++++ 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/mxsfb.yaml b/Documentation/devicetree/bindings/display/mxsfb.yaml index ec6533b1d4a3..d15bb8edc29f 100644 --- a/Documentation/devicetree/bindings/display/mxsfb.yaml +++ b/Documentation/devicetree/bindings/display/mxsfb.yaml @@ -58,6 +58,18 @@ properties: type: object

     properties:

•      data-shift:
  

Shouldn't that be bus-width ? -- Guido

•        enum: [16, 18, 24]
  

•        description: |
  

•          The output bus width. This value overrides the configuration
  

•          derived from the connected device (encoder or panel). It should
  

•          only be specified when PCB routing of the data signals require a
  

•          different bus width on the LCDIF and the connected device. For
  

•          instance, when a 18-bit RGB panel has its R[5:0], G[5:0] and
  

•          B[5:0] signals connected to LCD_DATA[7:2], LCD_DATA[15:10] and
  

•          LCD_DATA[23:18] instead of LCD_DATA[5:0], LCD_DATA[11:6] and
  

•          LCD_DATA[17:12], bus-width should be set to 24.
  

•      remote-endpoint:
         $ref: /schemas/types.yaml#/definitions/phandle
  
  

-- Regards,

Laurent Pinchart

Hi Laurent.

On Thu, Aug 13, 2020 at 04:29:05AM +0300, Laurent Pinchart wrote:

When the PCB routes the display data signals in an unconventional way, the output bus width may differ from the bus width of the connected panel or encoder. For instance, when a 18-bit RGB panel has its R[5:0], G[5:0] and B[5:0] signals connected to LCD_DATA[7:2], LCD_DATA[15:10] and LCD_DATA[23:18], the output bus width is 24 instead of 18 when the signals are routed to LCD_DATA[5:0], LCD_DATA[11:6] and LCD_DATA[17:12].

Add a bus-width property to describe this data routing.

Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

Some general comments. We have the bus format - for example MEDIA_BUS_FMT_RGB666_1X18. I was under the impression that the bus format defined the wiring used, so for example the bus format would tell if on used 18 bits as above. So with the bus format available the bus-width is not needed.

Today this detail is not expressed in DT but comes based on the compatible for the panel - so what this patch does is to add the bus format information to DT.

But then to do so would we not need to have something so we can specify all relevant MEDIA_BUS_FMT's? bus-width will not cut it.

Also the bus-width property (and data-shift property you accidentally reference) are both described in media/video-interfaces.txt. If we need bus-witdh - is it possible to re-use video-interfaces? It may need a conversion to yaml to get full validation, but a lot of .yaml files refer to the text file today so conversion can come later.

Sam

---

Documentation/devicetree/bindings/display/mxsfb.yaml | 12 ++++++++++++ 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/mxsfb.yaml b/Documentation/devicetree/bindings/display/mxsfb.yaml index ec6533b1d4a3..d15bb8edc29f 100644 --- a/Documentation/devicetree/bindings/display/mxsfb.yaml +++ b/Documentation/devicetree/bindings/display/mxsfb.yaml @@ -58,6 +58,18 @@ properties: type: object

     properties:

•      data-shift:
  

•        enum: [16, 18, 24]
  

•        description: |
  

•          The output bus width. This value overrides the configuration
  

•          derived from the connected device (encoder or panel). It should
  

•          only be specified when PCB routing of the data signals require a
  

•          different bus width on the LCDIF and the connected device. For
  

•          instance, when a 18-bit RGB panel has its R[5:0], G[5:0] and
  

•          B[5:0] signals connected to LCD_DATA[7:2], LCD_DATA[15:10] and
  

•          LCD_DATA[23:18] instead of LCD_DATA[5:0], LCD_DATA[11:6] and
  

•          LCD_DATA[17:12], bus-width should be set to 24.
  

•      remote-endpoint:
         $ref: /schemas/types.yaml#/definitions/phandle
  
  

-- Regards,

Laurent Pinchart

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On 2020-08-13 03:29, Laurent Pinchart wrote:

Rename the mxsfb.yaml binding schema to fsl,lcdif.yaml to match the usual bindings naming scheme.

I tend to prefer to just name it fsl,lcdif.yaml from the get-go.

If you prefer keeping it separate, then it should be patch 2 of the series.

-- Stefan

Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

.../devicetree/bindings/display/{mxsfb.yaml => fsl,lcdif.yaml} | 2 +- MAINTAINERS | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) rename Documentation/devicetree/bindings/display/{mxsfb.yaml => fsl,lcdif.yaml} (98%)

diff --git a/Documentation/devicetree/bindings/display/mxsfb.yaml b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml similarity index 98% rename from Documentation/devicetree/bindings/display/mxsfb.yaml rename to Documentation/devicetree/bindings/display/fsl,lcdif.yaml index d15bb8edc29f..60210775c31e 100644 --- a/Documentation/devicetree/bindings/display/mxsfb.yaml +++ b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml @@ -1,7 +1,7 @@ # SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2

---

-$id: http://devicetree.org/schemas/display/mxsfb.yaml# +$id: http://devicetree.org/schemas/display/fsl,lcdif.yaml# $schema: http://devicetree.org/meta-schemas/core.yaml#

title: Freescale/NXP i.MX LCD Interface (LCDIF) diff --git a/MAINTAINERS b/MAINTAINERS index e3fac23383d2..fe1bda639a39 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11757,7 +11757,7 @@ M: Stefan Agner stefan@agner.ch L: dri-devel@lists.freedesktop.org S: Supported T: git git://anongit.freedesktop.org/drm/drm-misc -F: Documentation/devicetree/bindings/display/mxsfb.yaml +F: Documentation/devicetree/bindings/display/fsl,lcdif.yaml F: drivers/gpu/drm/mxsfb/

MYLEX DAC960 PCI RAID Controller

Hi Sam,

On Sun, Aug 16, 2020 at 09:46:30AM +0200, Sam Ravnborg wrote:

On Thu, Aug 13, 2020 at 04:29:10AM +0300, Laurent Pinchart wrote:

...

A new bus-width DT property has been introduced in the bindings to allow overriding the bus width. Support it.

Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

We already reads the bus-width in following files in drm: atmel-hlcdc/atmel_hlcdc_output.c bridge/ti-tfp410.c

So this calls for a common helper to do this like:

int drm_of_bus_fmt(const struct device_node *ep) { }

This helper could then read bus-width, data-shift (if relevant) and return the relevant bus format.

I can see that bridge/ti-tfp410.c assumes 12 equals MEDIA_BUS_FMT_RGB888_2X12_LE where as mxsfb assumes 12 is MEDIA_BUS_FMT_RGB444_1X12. I do not know why neither how to handle this difference. Maybe this is something to do with DVI as this is what tfp410 seems to support.

I think the mapping from bus width to format is device-specific, I'm not sure a common helper is possible.

And now that I think about this, I wonder if data-shift = <2> wouldn't be a better option than bus-width = <24>, as a 18-bpp panel will always be connected with 18 lines, not 24. One issue, however, is that for the 565 case, I'd need a different data-shift value for R/B and for G, which isn't possible. I don't think that turning data-shift into a multi-integers property is worth it though. Given that DT properties are interpreted in the context of a particular binding using bus-width could be fine here, but I'm open to better alternatives if someone has a good proposal.

...

---

drivers/gpu/drm/mxsfb/mxsfb_drv.c | 26 ++++++++++++++++++++++++++ drivers/gpu/drm/mxsfb/mxsfb_drv.h | 2 ++ drivers/gpu/drm/mxsfb/mxsfb_kms.c | 8 ++++++-- 3 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c index 8c549c3931af..fab3aae8cf73 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c @@ -95,10 +95,36 @@ static int mxsfb_attach_bridge(struct mxsfb_drm_private *mxsfb) { struct drm_device *drm = mxsfb->drm; struct drm_connector_list_iter iter;

• struct device_node *ep; struct drm_panel *panel; struct drm_bridge *bridge;

• u32 bus_width = 0; int ret;

• ep = of_graph_get_endpoint_by_regs(drm->dev->of_node, 0, 0);

• if (!ep)

• return -ENODEV;
  

• of_property_read_u32(ep, "bus-width", &bus_width);

• of_node_put(ep);

• switch (bus_width) {

• case 16:

• mxsfb->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
  

• break;
  

• case 18:

• mxsfb->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
  

• break;
  

• case 24:

• mxsfb->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
  

• break;
  

• case 0:

• break;
  

• default:

• DRM_DEV_ERROR(drm->dev, "Invalid bus-width %u", bus_width);
  

• return -ENODEV;
  

• }

• ret = drm_of_find_panel_or_bridge(drm->dev->of_node, 0, 0, &panel, &bridge); if (ret)

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h b/drivers/gpu/drm/mxsfb/mxsfb_drv.h index 399d23e91ed1..c4f7a8a0c891 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h @@ -32,6 +32,8 @@ struct mxsfb_drm_private { struct clk *clk_axi; struct clk *clk_disp_axi;

• u32 bus_format;
• struct drm_device *drm; struct { struct drm_plane primary;

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c index b721b8b262ce..6d512f346918 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c @@ -50,11 +50,15 @@ static void mxsfb_set_formats(struct mxsfb_drm_private *mxsfb) { struct drm_device *drm = mxsfb->drm; const u32 format = mxsfb->crtc.primary->state->fb->format->format;

• u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;

• u32 bus_format; u32 ctrl, ctrl1;

• if (mxsfb->connector->display_info.num_bus_formats)

• if (mxsfb->bus_format)

• bus_format = mxsfb->bus_format;
  

• else if (mxsfb->connector->display_info.num_bus_formats) bus_format = mxsfb->connector->display_info.bus_formats[0];

• else

• bus_format = MEDIA_BUS_FMT_RGB888_1X24;
  
  

  DRM_DEV_DEBUG_DRIVER(drm->dev, "Using bus_format: 0x%08X\n", bus_format);