Himax HX8837 is a secondary display controller used to drive the panel
on OLPC platforms.
Signed-off-by: Lubomir Rintel <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
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 <[email protected]>
+%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 <[email protected]>
+
+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
+
+required:
+ - compatible
+ - reg
+ - load-gpios
+ - stat-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>;
+
+ 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>;
+ dcon_rgb_in: endpoint {
+ remote-endpoint = <&lcd0_rgb_out>;
+ };
+ };
+
+ port@1 {
+ reg = <0x01>;
+ dcon_gettl_out: endpoint {
+ remote-endpoint = <&panel_dettl_in>;
+ };
+ };
+ };
+ };
+ };
--
2.28.0
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 <[email protected]>
> Reviewed-by: Rob Herring <[email protected]>
>
> ---
> 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 <[email protected]>
> +%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 <[email protected]>
> +
> +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)
--
Regards,
Laurent Pinchart
On Sun, Nov 01, 2020 at 06:39:22PM +0200, Laurent Pinchart wrote:
> Hi Lubomir,
>
> Thank you for the patch.
Thanks for the message. Some responses inline below.
> 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 <[email protected]>
> > Reviewed-by: Rob Herring <[email protected]>
> >
> > ---
> > 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 <[email protected]>
> > +%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 <[email protected]>
> > +
> > +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 ?
There are four.
On the OLPC platform they're controlled together by the EC.
I've added the supplies to the EC driver and looked into supporting them
properly in the driver and am finding it somehow tricky to do it properly.
I couldn't figure out what is the proper place to enable and disable the
regulators. Also drm_bridge_remove() just mercilessly tearing down the
bridge without ensuring it's not used anymore doesn't help us on driver
unbind.
I'm wondering if it's okay if I leave the driver without explicit
support for the power supplies for now, assuming that EC just takes
care of enabling the power and never disable it?
> > +
> > +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 ?
Perhaps not, in theory.
Pretty sure the OLPC machines are the only ones that utilize this
silicon though.
> > + - 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.
Ugh, I was sure I ran checkpatch.pl, but apparently not.
Sorry for that.
> > + 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)
Okay, will add that.
Thank you
Lubo
>
> --
> Regards,
>
> Laurent Pinchart