Re: [PATCH v6 1/2] dt-bindings: display: himax,hx8837: Add Himax HX8837 bindings

Hi Lubomir,

Thank you for the patch.

On Fri, Oct 30, 2020 at 04:07:59AM +0100, Lubomir Rintel wrote:

Himax HX8837 is a secondary display controller used to drive the panel on OLPC platforms.

Signed-off-by: Lubomir Rintel lkundrak@v3.sk Reviewed-by: Rob Herring robh@kernel.org

---

Changes since v4:

• Rob's Reviewed-by

Changes since v3:

• Moved to bindings/display/
• Added the ports
• Converted to YAML
• Removed Pavel's Ack, because the changes are substantial

Changes since v2:

• s/betweend/between/

Changes since v1:

• s/load-gpio/load-gpios/
• Use interrupt bindings instead of gpio for the IRQ

.../bindings/display/bridge/himax,hx8837.yaml | 96 +++++++++++++++++++ 1 file changed, 96 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/bridge/himax,hx8837.yaml

diff --git a/Documentation/devicetree/bindings/display/bridge/himax,hx8837.yaml b/Documentation/devicetree/bindings/display/bridge/himax,hx8837.yaml new file mode 100644 index 0000000000000..f5b0a00f5089d --- /dev/null +++ b/Documentation/devicetree/bindings/display/bridge/himax,hx8837.yaml @@ -0,0 +1,96 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +# Copyright (C) 2018,2019,2020 Lubomir Rintel lkundrak@v3.sk +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/bridge/himax,hx8837.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml#

+title: HX8837 Display Controller Device Tree Bindings

+maintainers:

• • Lubomir Rintel lkundrak@v3.sk

+properties:

• compatible:
• const: himax,hx8837
• reg:
• const: 0xd
• load-gpios:
• maxItems: 1
• description: GPIO specifier of DCON_LOAD pin (active high)
• stat-gpios:
• minItems: 2
• description: GPIO specifier of DCON_STAT0 and DCON_STAT1 pins (active high)
• interrupts:
• maxItems: 1
• description: Interrupt specifier of DCON_IRQ pin (edge falling)
• ports:
• type: object
• properties:

•  port@0:
  

•    type: object
  

•    description: |
  

•      Video port for RGB input.
  

•  port@1:
  

•    type: object
  

•    description: |
  

•      Video port connected to the panel.
  

• required:

•  - port@0
  

•  - port@1
  

No regulators ?

+required:

• • compatible
• • reg
• • load-gpios
• • stat-gpios

Do stat-gpios need to be mandatory ? The driver in patch 2/2 doesn't seem to use them, could we have boards where those signals are not connected to GPIOs ?

• • interrupts
• • ports

+additionalProperties: false

+examples:

• • |
• #include <dt-bindings/gpio/gpio.h>
• #include <dt-bindings/interrupt-controller/irq.h>
• i2c {

•    #address-cells = <1>;
  

•    #size-cells = <0>;
  

Could you please avoid spaces or tabs at end of lines ? There are three other occurrences below.

•    lcd-controller@d {
  

•        compatible = "himax,hx8837";
  

•        reg = <0x0d>;
  

•        stat-gpios = <&gpio 100 GPIO_ACTIVE_HIGH>,
  

•                     <&gpio 101 GPIO_ACTIVE_HIGH>;
  

•        load-gpios = <&gpio 142 GPIO_ACTIVE_HIGH>;
  

•        interrupts = <&gpio 124 IRQ_TYPE_EDGE_FALLING>;
  

•        ports {
  

•            #address-cells = <0x01>;
  

•            #size-cells = <0x00>;
  

•            port@0 {
  

•                reg = <0x00>;
  

reg = <0> should be fine. Same below.

With thse small issues addressed,

•                dcon_rgb_in: endpoint {
  

•                    remote-endpoint = <&lcd0_rgb_out>;
  

•                };
  

•            };
  

•            port@1 {
  

•                reg = <0x01>;
  

•                dcon_gettl_out: endpoint {
  

•                    remote-endpoint = <&panel_dettl_in>;
  

•                };
  

•            };
  

•        };
  

•    };
  

• };

It's customary to end bindings with

...

(not sure why though, given that it seems to work find without)