* (no subject) @ 2020-10-30 3:07 Lubomir Rintel 2020-10-30 3:07 ` [PATCH v6 1/2] dt-bindings: display: himax,hx8837: Add Himax HX8837 bindings Lubomir Rintel 2020-10-30 3:08 ` [PATCH v6 2/2] drm/bridge: hx8837: add a Himax HX8837 display controller driver Lubomir Rintel 0 siblings, 2 replies; 8+ messages in thread From: Lubomir Rintel @ 2020-10-30 3:07 UTC (permalink / raw) To: Andrzej Hajda Cc: Daniel Vetter, David Airlie, Rob Herring, Neil Armstrong, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, dri-devel, devicetree, linux-kernel, Rob Herring, Sam Ravnborg Subject: [PATCH v6 0/2] Add a Himax HX8837 display controller driver Date: Sat, 26 Sep 2020 02:07:17 +0200 Message-ID: <20200926000719.229204-1-lkundrak@v3.sk> (raw) Hi, please take a look at the patches chained to this messages and consider applying them. They add support for the controller that drives the panel on the OLPC XO laptops. Compared to v5, points risen in review by Sam Ravnborg have been addressed. Details in change log of patch 2/2. Tested on an OLPC XO-1.75 laptop. Thank you Lubo ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v6 1/2] dt-bindings: display: himax,hx8837: Add Himax HX8837 bindings 2020-10-30 3:07 Lubomir Rintel @ 2020-10-30 3:07 ` Lubomir Rintel 2020-11-01 16:39 ` Laurent Pinchart 2020-10-30 3:08 ` [PATCH v6 2/2] drm/bridge: hx8837: add a Himax HX8837 display controller driver Lubomir Rintel 1 sibling, 1 reply; 8+ messages in thread From: Lubomir Rintel @ 2020-10-30 3:07 UTC (permalink / raw) To: Andrzej Hajda Cc: Daniel Vetter, David Airlie, Rob Herring, Neil Armstrong, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, dri-devel, devicetree, linux-kernel, Rob Herring, Sam Ravnborg, Lubomir Rintel 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 + +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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v6 1/2] dt-bindings: display: himax,hx8837: Add Himax HX8837 bindings 2020-10-30 3:07 ` [PATCH v6 1/2] dt-bindings: display: himax,hx8837: Add Himax HX8837 bindings Lubomir Rintel @ 2020-11-01 16:39 ` Laurent Pinchart 2020-11-18 20:34 ` Lubomir Rintel 0 siblings, 1 reply; 8+ messages in thread From: Laurent Pinchart @ 2020-11-01 16:39 UTC (permalink / raw) To: Lubomir Rintel Cc: Andrzej Hajda, Daniel Vetter, David Airlie, Rob Herring, Neil Armstrong, Jonas Karlman, Jernej Skrabec, dri-devel, devicetree, linux-kernel, Rob Herring, Sam Ravnborg 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) -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v6 1/2] dt-bindings: display: himax,hx8837: Add Himax HX8837 bindings 2020-11-01 16:39 ` Laurent Pinchart @ 2020-11-18 20:34 ` Lubomir Rintel 0 siblings, 0 replies; 8+ messages in thread From: Lubomir Rintel @ 2020-11-18 20:34 UTC (permalink / raw) To: Laurent Pinchart Cc: Andrzej Hajda, Daniel Vetter, David Airlie, Rob Herring, Neil Armstrong, Jonas Karlman, Jernej Skrabec, dri-devel, devicetree, linux-kernel, Rob Herring, Sam Ravnborg 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 <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 ? 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v6 2/2] drm/bridge: hx8837: add a Himax HX8837 display controller driver 2020-10-30 3:07 Lubomir Rintel 2020-10-30 3:07 ` [PATCH v6 1/2] dt-bindings: display: himax,hx8837: Add Himax HX8837 bindings Lubomir Rintel @ 2020-10-30 3:08 ` Lubomir Rintel 2020-10-31 8:01 ` Sam Ravnborg 1 sibling, 1 reply; 8+ messages in thread From: Lubomir Rintel @ 2020-10-30 3:08 UTC (permalink / raw) To: Andrzej Hajda Cc: Daniel Vetter, David Airlie, Rob Herring, Neil Armstrong, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, dri-devel, devicetree, linux-kernel, Rob Herring, Sam Ravnborg, Lubomir Rintel Himax HX8837 is used to drive the LCD panel on OLPC platforms. It controls the panel backlight and is able to refresh it when the LCD controller (and the rest of the plaform) is powered off. It also converts regular RGB color data from the LCDC so that it looks reasonable on the OLPC LCD panel with a monochromatic layer on top of a layer that can either reflect light (b/w sunlight readable mode) or light pattern of red, green and blue pixels. At this point, the driver is rather basic. The self-refresh mode is not supported. There's no way of independently controlling the color swizzling, antialiasing or b/w conversion, but it probably isn't too useful either. There's another driver for the same hardware on OLPC XO-1.5 and XO-1.75 in drivers/staging/olpc_dcon. The display on that hardware doesn't utilize DRM, so this driver doesn't replace the other one yet. Signed-off-by: Lubomir Rintel <lkundrak@v3.sk> --- Changes since v5: (All based on feedback from Sam Ravnborg) - Fix indentation in Kconfig - Sort #includes - Use a constant for max brightness instead of a literal - Remove struct drm_panel from priv data - Use dev_err() instead of DRM_ERROR - Replace direct use of backlight props.brightness with backlight_get_brightness() - Document sentinels with { /* sentinel */ } - Remove unsetting of panel->backlight Changes since v3: - Added this patch, in place of a driver derived from drivers/staging/olpc_dcon. Compared to the previous one this implements the bare minimum, without the fancy stuff such as self-refresh that need more work/thinking. drivers/gpu/drm/bridge/Kconfig | 13 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/himax-hx8837.c | 330 ++++++++++++++++++++++++++ 3 files changed, 344 insertions(+) create mode 100644 drivers/gpu/drm/bridge/himax-hx8837.c diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index ef91646441b16..881780719af7c 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -48,6 +48,19 @@ config DRM_DISPLAY_CONNECTOR on ARM-based platforms. Saying Y here when this driver is not needed will not cause any issue. +config DRM_HIMAX_HX8837 + tristate "HiMax HX8837 OLPC Display Controller" + depends on OF + depends on OLPC || ARCH_MMP || COMPILE_TEST + select DRM_KMS_HELPER + select BACKLIGHT_LCD_SUPPORT + select BACKLIGHT_CLASS_DEVICE + help + Enable support for HiMax HX8837 Display Controller as found in the + OLPC XO laptops. + + If your laptop doesn't have green ears, say "N" + config DRM_LONTIUM_LT9611 tristate "Lontium LT9611 DSI/HDMI bridge" select SND_SOC_HDMI_CODEC if SND_SOC diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile index 2b3aff104e466..21f72df3260db 100644 --- a/drivers/gpu/drm/bridge/Makefile +++ b/drivers/gpu/drm/bridge/Makefile @@ -2,6 +2,7 @@ obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o obj-$(CONFIG_DRM_CHRONTEL_CH7033) += chrontel-ch7033.o obj-$(CONFIG_DRM_DISPLAY_CONNECTOR) += display-connector.o +obj-$(CONFIG_DRM_HIMAX_HX8837) += himax-hx8837.o obj-$(CONFIG_DRM_LONTIUM_LT9611) += lontium-lt9611.o obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o diff --git a/drivers/gpu/drm/bridge/himax-hx8837.c b/drivers/gpu/drm/bridge/himax-hx8837.c new file mode 100644 index 0000000000000..f472e16cc331d --- /dev/null +++ b/drivers/gpu/drm/bridge/himax-hx8837.c @@ -0,0 +1,330 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * HiMax HX8837 Display Controller Driver + * + * Datasheet: http://wiki.laptop.org/images/0/09/DCON_datasheet_HX8837-A.pdf + * + * Copyright (C) 2020 Lubomir Rintel + */ + +#include <drm/drm_atomic_helper.h> +#include <drm/drm_bridge.h> +#include <drm/drm_of.h> +#include <drm/drm_panel.h> +#include <drm/drm_print.h> +#include <drm/drm_probe_helper.h> + +#include <linux/gpio/consumer.h> +#include <linux/module.h> +#include <linux/regmap.h> + +#define bridge_to_hx8837_priv(x) \ + container_of(x, struct hx8837_priv, bridge) + +/* DCON registers */ +enum { + DCON_REG_ID = 0x00, + DCON_REG_MODE = 0x01, + DCON_REG_HRES = 0x02, + DCON_REG_HTOTAL = 0x03, + DCON_REG_HSYNC_WIDTH = 0x04, + DCON_REG_VRES = 0x05, + DCON_REG_VTOTAL = 0x06, + DCON_REG_VSYNC_WIDTH = 0x07, + DCON_REG_TIMEOUT = 0x08, + DCON_REG_SCAN_INT = 0x09, + DCON_REG_BRIGHT = 0x0a, + DCON_REG_MEM_OPT_A = 0x41, + DCON_REG_MEM_OPT_B = 0x42, +}; + +/* DCON_REG_MODE */ +enum { + MODE_PASSTHRU = BIT(0), + MODE_SLEEP = BIT(1), + MODE_SLEEP_AUTO = BIT(2), + MODE_BL_ENABLE = BIT(3), + MODE_BLANK = BIT(4), + MODE_CSWIZZLE = BIT(5), + MODE_COL_AA = BIT(6), + MODE_MONO_LUMA = BIT(7), + MODE_SCAN_INT = BIT(8), + MODE_CLOCKDIV = BIT(9), + MODE_DEBUG = BIT(14), + MODE_SELFTEST = BIT(15), +}; + +/* DCON_REG_BRIGHT */ +enum { + BRIGHT_MASK = GENMASK(7, 0), +}; + +struct hx8837_priv { + struct device *dev; + struct regmap *regmap; + struct gpio_desc *load_gpio; + + struct drm_bridge *panel_bridge; + struct drm_bridge bridge; +}; + +static int hx8837_bridge_attach(struct drm_bridge *bridge, + enum drm_bridge_attach_flags flags) +{ + struct hx8837_priv *priv = bridge_to_hx8837_priv(bridge); + + return drm_bridge_attach(bridge->encoder, priv->panel_bridge, + bridge, flags); +} + +static enum drm_mode_status hx8837_bridge_mode_valid( + struct drm_bridge *bridge, + const struct drm_display_info *info, + const struct drm_display_mode *mode) +{ + if (mode->hdisplay > 0xffff) + return MODE_BAD_HVALUE; + if (mode->htotal > 0xffff) + return MODE_BAD_HVALUE; + if (mode->hsync_start - mode->hdisplay > 0xff) + return MODE_HBLANK_WIDE; + if (mode->hsync_end - mode->hsync_start > 0xff) + return MODE_HSYNC_WIDE; + if (mode->vdisplay > 0xffff) + return MODE_BAD_VVALUE; + if (mode->vtotal > 0xffff) + return MODE_BAD_VVALUE; + if (mode->vsync_start - mode->vdisplay > 0xff) + return MODE_VBLANK_WIDE; + if (mode->vsync_end - mode->vsync_start > 0xff) + return MODE_VSYNC_WIDE; + + return MODE_OK; +} + +static void hx8837_bridge_disable(struct drm_bridge *bridge) +{ + struct hx8837_priv *priv = bridge_to_hx8837_priv(bridge); + int ret; + + ret = gpiod_direction_output(priv->load_gpio, 0); + if (ret) + dev_err(priv->dev, "error enabling the dcon load: %d\n", ret); + + ret = regmap_update_bits(priv->regmap, DCON_REG_MODE, + MODE_PASSTHRU | + MODE_SLEEP, + MODE_PASSTHRU | + MODE_SLEEP); + if (ret) + dev_err(priv->dev, "error disabling the dcon: %d\n", ret); +} + +static void hx8837_bridge_enable(struct drm_bridge *bridge) +{ + struct hx8837_priv *priv = bridge_to_hx8837_priv(bridge); + int ret; + + ret = regmap_update_bits(priv->regmap, DCON_REG_MODE, + MODE_PASSTHRU | + MODE_SLEEP | + MODE_SLEEP_AUTO | + MODE_BLANK | + MODE_SCAN_INT | + MODE_CLOCKDIV | + MODE_DEBUG | + MODE_SELFTEST, + MODE_PASSTHRU); + if (ret) + dev_err(priv->dev, "error enabling the dcon: %d\n", ret); + + ret = gpiod_direction_output(priv->load_gpio, 1); + if (ret) + dev_err(priv->dev, "error enabling the dcon load: %d\n", ret); +} + +static void hx8837_bridge_mode_set(struct drm_bridge *bridge, + const struct drm_display_mode *mode, + const struct drm_display_mode *adjusted_mode) +{ + struct hx8837_priv *priv = bridge_to_hx8837_priv(bridge); + + regmap_write(priv->regmap, DCON_REG_HRES, mode->hdisplay); + regmap_write(priv->regmap, DCON_REG_HTOTAL, mode->htotal); + regmap_write(priv->regmap, DCON_REG_HSYNC_WIDTH, + (mode->hsync_start - mode->hdisplay) << 8 | + (mode->hsync_end - mode->hsync_start)); + regmap_write(priv->regmap, DCON_REG_VRES, mode->vdisplay); + regmap_write(priv->regmap, DCON_REG_VTOTAL, mode->vtotal); + regmap_write(priv->regmap, DCON_REG_VSYNC_WIDTH, + (mode->vsync_start - mode->vdisplay) << 8 | + (mode->vsync_end - mode->vsync_start)); +} + +static const struct drm_bridge_funcs hx8837_bridge_funcs = { + .attach = hx8837_bridge_attach, + .mode_valid = hx8837_bridge_mode_valid, + .disable = hx8837_bridge_disable, + .enable = hx8837_bridge_enable, + .mode_set = hx8837_bridge_mode_set, +}; + +static int hx8837_bl_update_status(struct backlight_device *bl) +{ + struct hx8837_priv *priv = bl_get_data(bl); + unsigned int val; + int ret; + + ret = regmap_update_bits(priv->regmap, DCON_REG_BRIGHT, + BRIGHT_MASK, + backlight_get_brightness(bl)); + if (ret) { + dev_err(&bl->dev, "error setting the backlight: %d\n", ret); + return ret; + } + + if (backlight_get_brightness(bl)) + val = MODE_CSWIZZLE | MODE_COL_AA; + else + val = MODE_MONO_LUMA; + + ret = regmap_update_bits(priv->regmap, DCON_REG_MODE, + MODE_CSWIZZLE | + MODE_COL_AA | + MODE_MONO_LUMA, + val); + if (ret) { + dev_err(&bl->dev, "error setting color mode: %d\n", ret); + return ret; + } + + return 0; +} + +static const struct backlight_ops hx8837_bl_ops = { + .update_status = hx8837_bl_update_status, +}; + +static const struct regmap_config hx8837_regmap_config = { + .reg_bits = 8, + .val_bits = 16, + .max_register = 0x4c, + .val_format_endian = REGMAP_ENDIAN_LITTLE, +}; + +static int hx8837_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + struct backlight_properties bl_props = { + .type = BACKLIGHT_RAW, + .max_brightness = BRIGHT_MASK, + }; + struct device *dev = &client->dev; + struct backlight_device *bl; + struct hx8837_priv *priv; + struct drm_panel *panel; + unsigned int val; + int ret; + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + dev_set_drvdata(dev, priv); + priv->dev = dev; + + priv->load_gpio = devm_gpiod_get(dev, "load", GPIOD_ASIS); + if (IS_ERR(priv->load_gpio)) + return PTR_ERR(priv->load_gpio); + + ret = drm_of_find_panel_or_bridge(dev->of_node, 1, -1, &panel, NULL); + if (ret) + return ret; + + if (panel->backlight) { + dev_err(dev, "the panel already has a backlight controller\n"); + return -ENODEV; + } + + priv->panel_bridge = devm_drm_panel_bridge_add(dev, panel); + if (IS_ERR(priv->panel_bridge)) + return PTR_ERR(priv->panel_bridge); + + priv->regmap = devm_regmap_init_i2c(client, &hx8837_regmap_config); + if (IS_ERR(priv->regmap)) { + dev_err(dev, "regmap init failed\n"); + return PTR_ERR(priv->regmap); + } + + ret = regmap_read(priv->regmap, DCON_REG_ID, &val); + if (ret < 0) { + dev_err(dev, "error reading the model id: %d\n", ret); + return ret; + } + if ((val & 0xff00) != 0xdc00) { + dev_err(dev, "the device is not a hx8837\n"); + return -ENODEV; + } + + ret = regmap_read(priv->regmap, DCON_REG_BRIGHT, &val); + if (ret < 0) { + dev_err(&bl->dev, "error getting the backlight: %d\n", ret); + return ret; + } + bl_props.brightness = val & 0xf; + + bl = devm_backlight_device_register(dev, dev_name(dev), dev, priv, + &hx8837_bl_ops, &bl_props); + if (IS_ERR(bl)) { + dev_err(dev, "failed to register backlight\n"); + return PTR_ERR(bl); + } + + panel->backlight = bl; + + INIT_LIST_HEAD(&priv->bridge.list); + priv->bridge.funcs = &hx8837_bridge_funcs; + priv->bridge.of_node = dev->of_node; + drm_bridge_add(&priv->bridge); + + dev_info(dev, "HiMax HX8837 Display Controller Driver\n"); + return 0; +} + +static int hx8837_remove(struct i2c_client *client) +{ + struct device *dev = &client->dev; + struct hx8837_priv *priv = dev_get_drvdata(dev); + + drm_bridge_remove(&priv->bridge); + + return 0; +} + +static const struct of_device_id hx8837_dt_ids[] = { + { .compatible = "himax,hx8837", }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, hx8837_dt_ids); + +static const struct i2c_device_id hx8837_ids[] = { + { "hx8837", 0 }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(i2c, hx8837_ids); + +static struct i2c_driver hx8837_driver = { + .probe = hx8837_probe, + .remove = hx8837_remove, + .driver = { + .name = "hx8837", + .of_match_table = of_match_ptr(hx8837_dt_ids), + }, + .id_table = hx8837_ids, +}; + +module_i2c_driver(hx8837_driver); + +MODULE_AUTHOR("Lubomir Rintel <lkundrak@v3.sk>"); +MODULE_DESCRIPTION("HiMax HX8837 Display Controller Driver"); +MODULE_LICENSE("GPL"); -- 2.28.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v6 2/2] drm/bridge: hx8837: add a Himax HX8837 display controller driver 2020-10-30 3:08 ` [PATCH v6 2/2] drm/bridge: hx8837: add a Himax HX8837 display controller driver Lubomir Rintel @ 2020-10-31 8:01 ` Sam Ravnborg 2020-10-31 20:12 ` Lubomir Rintel 0 siblings, 1 reply; 8+ messages in thread From: Sam Ravnborg @ 2020-10-31 8:01 UTC (permalink / raw) To: Lubomir Rintel Cc: Andrzej Hajda, Daniel Vetter, David Airlie, Rob Herring, Neil Armstrong, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, dri-devel, devicetree, linux-kernel, Rob Herring Hi Lubomir. On Fri, Oct 30, 2020 at 04:08:00AM +0100, Lubomir Rintel wrote: > Himax HX8837 is used to drive the LCD panel on OLPC platforms. > > It controls the panel backlight and is able to refresh it when the LCD > controller (and the rest of the plaform) is powered off. > > It also converts regular RGB color data from the LCDC so that it looks > reasonable on the OLPC LCD panel with a monochromatic layer on top of a > layer that can either reflect light (b/w sunlight readable mode) or light > pattern of red, green and blue pixels. > > At this point, the driver is rather basic. The self-refresh mode is not > supported. There's no way of independently controlling the color swizzling, > antialiasing or b/w conversion, but it probably isn't too useful either. > > There's another driver for the same hardware on OLPC XO-1.5 and XO-1.75 > in drivers/staging/olpc_dcon. The display on that hardware doesn't utilize > DRM, so this driver doesn't replace the other one yet. > > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk> > > --- > Changes since v5: > (All based on feedback from Sam Ravnborg) > - Fix indentation in Kconfig > - Sort #includes > - Use a constant for max brightness instead of a literal > - Remove struct drm_panel from priv data > - Use dev_err() instead of DRM_ERROR > - Replace direct use of backlight props.brightness with > backlight_get_brightness() > - Document sentinels with { /* sentinel */ } > - Remove unsetting of panel->backlight Thanks, I missed a few things during the last round, so here is a few more comments. Only very trivial things but lets get them fixed before we add the driver to drm-misc-next. Sam > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index ef91646441b16..881780719af7c 100644 > --- a/drivers/gpu/drm/bridge/Kconfig > +++ b/drivers/gpu/drm/bridge/Kconfig > @@ -48,6 +48,19 @@ config DRM_DISPLAY_CONNECTOR > on ARM-based platforms. Saying Y here when this driver is not needed > will not cause any issue. > > +config DRM_HIMAX_HX8837 > + tristate "HiMax HX8837 OLPC Display Controller" > + depends on OF > + depends on OLPC || ARCH_MMP || COMPILE_TEST > + select DRM_KMS_HELPER > + select BACKLIGHT_LCD_SUPPORT Unknown symbol - "git grep BACKLIGHT_LCD_SUPPORT" only turned up one unrelated hit. > + select BACKLIGHT_CLASS_DEVICE Please use a depends - using select on a symbol with a prompt is always wrong. Yeah, I know you then need to enable backlight to see this driver. Sorry, but this is the best we can do now. Many other drivers can cope with depends here. > + help > + Enable support for HiMax HX8837 Display Controller as found in the > + OLPC XO laptops. > + > + If your laptop doesn't have green ears, say "N" I like this last comment :-) > + > config DRM_LONTIUM_LT9611 > tristate "Lontium LT9611 DSI/HDMI bridge" > select SND_SOC_HDMI_CODEC if SND_SOC > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile > index 2b3aff104e466..21f72df3260db 100644 > --- a/drivers/gpu/drm/bridge/Makefile > +++ b/drivers/gpu/drm/bridge/Makefile > @@ -2,6 +2,7 @@ > obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o > obj-$(CONFIG_DRM_CHRONTEL_CH7033) += chrontel-ch7033.o > obj-$(CONFIG_DRM_DISPLAY_CONNECTOR) += display-connector.o > +obj-$(CONFIG_DRM_HIMAX_HX8837) += himax-hx8837.o > obj-$(CONFIG_DRM_LONTIUM_LT9611) += lontium-lt9611.o > obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o > obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o > diff --git a/drivers/gpu/drm/bridge/himax-hx8837.c b/drivers/gpu/drm/bridge/himax-hx8837.c > new file mode 100644 > index 0000000000000..f472e16cc331d > --- /dev/null > +++ b/drivers/gpu/drm/bridge/himax-hx8837.c > @@ -0,0 +1,330 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * HiMax HX8837 Display Controller Driver > + * > + * Datasheet: http://wiki.laptop.org/images/0/09/DCON_datasheet_HX8837-A.pdf > + * > + * Copyright (C) 2020 Lubomir Rintel > + */ > + > +#include <drm/drm_atomic_helper.h> > +#include <drm/drm_bridge.h> > +#include <drm/drm_of.h> > +#include <drm/drm_panel.h> > +#include <drm/drm_print.h> drm_print.h is no longer used and should be dropped. > +#include <drm/drm_probe_helper.h> > + > +#include <linux/gpio/consumer.h> > +#include <linux/module.h> > +#include <linux/regmap.h> > + > +#define bridge_to_hx8837_priv(x) \ > + container_of(x, struct hx8837_priv, bridge) > + > +/* DCON registers */ > +enum { > + DCON_REG_ID = 0x00, > + DCON_REG_MODE = 0x01, > + DCON_REG_HRES = 0x02, > + DCON_REG_HTOTAL = 0x03, > + DCON_REG_HSYNC_WIDTH = 0x04, > + DCON_REG_VRES = 0x05, > + DCON_REG_VTOTAL = 0x06, > + DCON_REG_VSYNC_WIDTH = 0x07, > + DCON_REG_TIMEOUT = 0x08, > + DCON_REG_SCAN_INT = 0x09, > + DCON_REG_BRIGHT = 0x0a, > + DCON_REG_MEM_OPT_A = 0x41, > + DCON_REG_MEM_OPT_B = 0x42, > +}; > + > +/* DCON_REG_MODE */ > +enum { > + MODE_PASSTHRU = BIT(0), > + MODE_SLEEP = BIT(1), > + MODE_SLEEP_AUTO = BIT(2), > + MODE_BL_ENABLE = BIT(3), > + MODE_BLANK = BIT(4), > + MODE_CSWIZZLE = BIT(5), > + MODE_COL_AA = BIT(6), > + MODE_MONO_LUMA = BIT(7), > + MODE_SCAN_INT = BIT(8), > + MODE_CLOCKDIV = BIT(9), > + MODE_DEBUG = BIT(14), > + MODE_SELFTEST = BIT(15), > +}; > + > +/* DCON_REG_BRIGHT */ > +enum { > + BRIGHT_MASK = GENMASK(7, 0), > +}; > + > +struct hx8837_priv { > + struct device *dev; > + struct regmap *regmap; > + struct gpio_desc *load_gpio; > + > + struct drm_bridge *panel_bridge; > + struct drm_bridge bridge; > +}; > + > +static int hx8837_bridge_attach(struct drm_bridge *bridge, > + enum drm_bridge_attach_flags flags) > +{ > + struct hx8837_priv *priv = bridge_to_hx8837_priv(bridge); > + > + return drm_bridge_attach(bridge->encoder, priv->panel_bridge, > + bridge, flags); > +} > + > +static enum drm_mode_status hx8837_bridge_mode_valid( > + struct drm_bridge *bridge, > + const struct drm_display_info *info, > + const struct drm_display_mode *mode) > +{ > + if (mode->hdisplay > 0xffff) > + return MODE_BAD_HVALUE; > + if (mode->htotal > 0xffff) > + return MODE_BAD_HVALUE; > + if (mode->hsync_start - mode->hdisplay > 0xff) > + return MODE_HBLANK_WIDE; > + if (mode->hsync_end - mode->hsync_start > 0xff) > + return MODE_HSYNC_WIDE; > + if (mode->vdisplay > 0xffff) > + return MODE_BAD_VVALUE; > + if (mode->vtotal > 0xffff) > + return MODE_BAD_VVALUE; > + if (mode->vsync_start - mode->vdisplay > 0xff) > + return MODE_VBLANK_WIDE; > + if (mode->vsync_end - mode->vsync_start > 0xff) > + return MODE_VSYNC_WIDE; > + > + return MODE_OK; > +} > + > +static void hx8837_bridge_disable(struct drm_bridge *bridge) > +{ > + struct hx8837_priv *priv = bridge_to_hx8837_priv(bridge); > + int ret; > + > + ret = gpiod_direction_output(priv->load_gpio, 0); > + if (ret) > + dev_err(priv->dev, "error enabling the dcon load: %d\n", ret); I assume this is disabling the dcon load - so text needs an update > + > + ret = regmap_update_bits(priv->regmap, DCON_REG_MODE, > + MODE_PASSTHRU | > + MODE_SLEEP, > + MODE_PASSTHRU | > + MODE_SLEEP); > + if (ret) > + dev_err(priv->dev, "error disabling the dcon: %d\n", ret); > +} > + > +static void hx8837_bridge_enable(struct drm_bridge *bridge) > +{ > + struct hx8837_priv *priv = bridge_to_hx8837_priv(bridge); > + int ret; > + > + ret = regmap_update_bits(priv->regmap, DCON_REG_MODE, > + MODE_PASSTHRU | > + MODE_SLEEP | > + MODE_SLEEP_AUTO | > + MODE_BLANK | > + MODE_SCAN_INT | > + MODE_CLOCKDIV | > + MODE_DEBUG | > + MODE_SELFTEST, > + MODE_PASSTHRU); > + if (ret) > + dev_err(priv->dev, "error enabling the dcon: %d\n", ret); > + > + ret = gpiod_direction_output(priv->load_gpio, 1); > + if (ret) > + dev_err(priv->dev, "error enabling the dcon load: %d\n", ret); > +} > + > +static void hx8837_bridge_mode_set(struct drm_bridge *bridge, > + const struct drm_display_mode *mode, > + const struct drm_display_mode *adjusted_mode) > +{ > + struct hx8837_priv *priv = bridge_to_hx8837_priv(bridge); > + > + regmap_write(priv->regmap, DCON_REG_HRES, mode->hdisplay); > + regmap_write(priv->regmap, DCON_REG_HTOTAL, mode->htotal); > + regmap_write(priv->regmap, DCON_REG_HSYNC_WIDTH, > + (mode->hsync_start - mode->hdisplay) << 8 | > + (mode->hsync_end - mode->hsync_start)); > + regmap_write(priv->regmap, DCON_REG_VRES, mode->vdisplay); > + regmap_write(priv->regmap, DCON_REG_VTOTAL, mode->vtotal); > + regmap_write(priv->regmap, DCON_REG_VSYNC_WIDTH, > + (mode->vsync_start - mode->vdisplay) << 8 | > + (mode->vsync_end - mode->vsync_start)); > +} > + > +static const struct drm_bridge_funcs hx8837_bridge_funcs = { > + .attach = hx8837_bridge_attach, > + .mode_valid = hx8837_bridge_mode_valid, > + .disable = hx8837_bridge_disable, > + .enable = hx8837_bridge_enable, > + .mode_set = hx8837_bridge_mode_set, > +}; > + > +static int hx8837_bl_update_status(struct backlight_device *bl) > +{ > + struct hx8837_priv *priv = bl_get_data(bl); > + unsigned int val; > + int ret; > + > + ret = regmap_update_bits(priv->regmap, DCON_REG_BRIGHT, > + BRIGHT_MASK, > + backlight_get_brightness(bl)); > + if (ret) { > + dev_err(&bl->dev, "error setting the backlight: %d\n", ret); > + return ret; > + } > + > + if (backlight_get_brightness(bl)) > + val = MODE_CSWIZZLE | MODE_COL_AA; > + else > + val = MODE_MONO_LUMA; > + > + ret = regmap_update_bits(priv->regmap, DCON_REG_MODE, > + MODE_CSWIZZLE | > + MODE_COL_AA | > + MODE_MONO_LUMA, > + val); > + if (ret) { > + dev_err(&bl->dev, "error setting color mode: %d\n", ret); > + return ret; > + } > + > + return 0; > +} > + > +static const struct backlight_ops hx8837_bl_ops = { > + .update_status = hx8837_bl_update_status, > +}; > + > +static const struct regmap_config hx8837_regmap_config = { > + .reg_bits = 8, > + .val_bits = 16, > + .max_register = 0x4c, > + .val_format_endian = REGMAP_ENDIAN_LITTLE, > +}; > + > +static int hx8837_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct backlight_properties bl_props = { > + .type = BACKLIGHT_RAW, > + .max_brightness = BRIGHT_MASK, > + }; I have recently learned we shall also always set the scale. This is missing in many drivers - but please do it here. > + struct device *dev = &client->dev; > + struct backlight_device *bl; > + struct hx8837_priv *priv; > + struct drm_panel *panel; > + unsigned int val; > + int ret; > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + dev_set_drvdata(dev, priv); > + priv->dev = dev; > + > + priv->load_gpio = devm_gpiod_get(dev, "load", GPIOD_ASIS); > + if (IS_ERR(priv->load_gpio)) > + return PTR_ERR(priv->load_gpio); > + > + ret = drm_of_find_panel_or_bridge(dev->of_node, 1, -1, &panel, NULL); > + if (ret) > + return ret; > + > + if (panel->backlight) { > + dev_err(dev, "the panel already has a backlight controller\n"); > + return -ENODEV; > + } > + > + priv->panel_bridge = devm_drm_panel_bridge_add(dev, panel); > + if (IS_ERR(priv->panel_bridge)) > + return PTR_ERR(priv->panel_bridge); > + > + priv->regmap = devm_regmap_init_i2c(client, &hx8837_regmap_config); > + if (IS_ERR(priv->regmap)) { > + dev_err(dev, "regmap init failed\n"); regmap_init_i2c failed? > + return PTR_ERR(priv->regmap); > + } > + > + ret = regmap_read(priv->regmap, DCON_REG_ID, &val); > + if (ret < 0) { > + dev_err(dev, "error reading the model id: %d\n", ret); > + return ret; > + } > + if ((val & 0xff00) != 0xdc00) { > + dev_err(dev, "the device is not a hx8837\n"); > + return -ENODEV; > + } > + > + ret = regmap_read(priv->regmap, DCON_REG_BRIGHT, &val); > + if (ret < 0) { > + dev_err(&bl->dev, "error getting the backlight: %d\n", ret); > + return ret; > + } > + bl_props.brightness = val & 0xf; > + > + bl = devm_backlight_device_register(dev, dev_name(dev), dev, priv, > + &hx8837_bl_ops, &bl_props); > + if (IS_ERR(bl)) { > + dev_err(dev, "failed to register backlight\n"); > + return PTR_ERR(bl); > + } > + > + panel->backlight = bl; > + > + INIT_LIST_HEAD(&priv->bridge.list); > + priv->bridge.funcs = &hx8837_bridge_funcs; > + priv->bridge.of_node = dev->of_node; > + drm_bridge_add(&priv->bridge); > + > + dev_info(dev, "HiMax HX8837 Display Controller Driver\n"); Debug - not needed > + return 0; > +} > + > +static int hx8837_remove(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + struct hx8837_priv *priv = dev_get_drvdata(dev); > + > + drm_bridge_remove(&priv->bridge); > + > + return 0; > +} > + > +static const struct of_device_id hx8837_dt_ids[] = { > + { .compatible = "himax,hx8837", }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, hx8837_dt_ids); > + > +static const struct i2c_device_id hx8837_ids[] = { > + { "hx8837", 0 }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(i2c, hx8837_ids); > + > +static struct i2c_driver hx8837_driver = { > + .probe = hx8837_probe, > + .remove = hx8837_remove, > + .driver = { > + .name = "hx8837", > + .of_match_table = of_match_ptr(hx8837_dt_ids), > + }, > + .id_table = hx8837_ids, > +}; > + > +module_i2c_driver(hx8837_driver); > + > +MODULE_AUTHOR("Lubomir Rintel <lkundrak@v3.sk>"); > +MODULE_DESCRIPTION("HiMax HX8837 Display Controller Driver"); > +MODULE_LICENSE("GPL"); > -- > 2.28.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v6 2/2] drm/bridge: hx8837: add a Himax HX8837 display controller driver 2020-10-31 8:01 ` Sam Ravnborg @ 2020-10-31 20:12 ` Lubomir Rintel 2020-10-31 20:56 ` Sam Ravnborg 0 siblings, 1 reply; 8+ messages in thread From: Lubomir Rintel @ 2020-10-31 20:12 UTC (permalink / raw) To: Sam Ravnborg Cc: Andrzej Hajda, Daniel Vetter, David Airlie, Rob Herring, Neil Armstrong, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, dri-devel, devicetree, linux-kernel, Rob Herring Hello Sam, thanks for your response. On Sat, Oct 31, 2020 at 09:01:37AM +0100, Sam Ravnborg wrote: > Hi Lubomir. > > On Fri, Oct 30, 2020 at 04:08:00AM +0100, Lubomir Rintel wrote: > > Himax HX8837 is used to drive the LCD panel on OLPC platforms. > > > > It controls the panel backlight and is able to refresh it when the LCD > > controller (and the rest of the plaform) is powered off. > > > > It also converts regular RGB color data from the LCDC so that it looks > > reasonable on the OLPC LCD panel with a monochromatic layer on top of a > > layer that can either reflect light (b/w sunlight readable mode) or light > > pattern of red, green and blue pixels. > > > > At this point, the driver is rather basic. The self-refresh mode is not > > supported. There's no way of independently controlling the color swizzling, > > antialiasing or b/w conversion, but it probably isn't too useful either. > > > > There's another driver for the same hardware on OLPC XO-1.5 and XO-1.75 > > in drivers/staging/olpc_dcon. The display on that hardware doesn't utilize > > DRM, so this driver doesn't replace the other one yet. > > > > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk> > > > > --- > > Changes since v5: > > (All based on feedback from Sam Ravnborg) > > - Fix indentation in Kconfig > > - Sort #includes > > - Use a constant for max brightness instead of a literal > > - Remove struct drm_panel from priv data > > - Use dev_err() instead of DRM_ERROR > > - Replace direct use of backlight props.brightness with > > backlight_get_brightness() > > - Document sentinels with { /* sentinel */ } > > - Remove unsetting of panel->backlight > > Thanks, I missed a few things during the last round, so here is a few > more comments. Only very trivial things but lets get them fixed before > we add the driver to drm-misc-next. > > Sam > > > > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > > index ef91646441b16..881780719af7c 100644 > > --- a/drivers/gpu/drm/bridge/Kconfig > > +++ b/drivers/gpu/drm/bridge/Kconfig > > @@ -48,6 +48,19 @@ config DRM_DISPLAY_CONNECTOR > > on ARM-based platforms. Saying Y here when this driver is not needed > > will not cause any issue. > > > > +config DRM_HIMAX_HX8837 > > + tristate "HiMax HX8837 OLPC Display Controller" > > + depends on OF > > + depends on OLPC || ARCH_MMP || COMPILE_TEST > > + select DRM_KMS_HELPER > > + select BACKLIGHT_LCD_SUPPORT > Unknown symbol - "git grep BACKLIGHT_LCD_SUPPORT" only turned up one > unrelated hit. > > > + select BACKLIGHT_CLASS_DEVICE > Please use a depends - using select on a symbol with a prompt is always > wrong. Yeah, I know you then need to enable backlight to see this > driver. Sorry, but this is the best we can do now. > Many other drivers can cope with depends here. This results in a dependency loop: drivers/video/fbdev/Kconfig:12:error: recursive dependency detected! drivers/video/fbdev/Kconfig:12: symbol FB is selected by DRM_KMS_FB_HELPER drivers/gpu/drm/Kconfig:80: symbol DRM_KMS_FB_HELPER depends on DRM_KMS_HELPER drivers/gpu/drm/Kconfig:74: symbol DRM_KMS_HELPER is selected by DRM_HIMAX_HX8837 drivers/gpu/drm/bridge/Kconfig:51: symbol DRM_HIMAX_HX8837 depends on BACKLIGHT_CLASS_DEVICE drivers/video/backlight/Kconfig:143: symbol BACKLIGHT_CLASS_DEVICE is selected by FB_BACKLIGHT drivers/video/fbdev/Kconfig:187: symbol FB_BACKLIGHT depends on FB Unfortunately I have no idea how to resolve it at the moment. I suppose I can look further into it if necessary. Or is it okay if I leave it at select BACKLIGHT_CLASS_DEVICE for now? > > + help > > + Enable support for HiMax HX8837 Display Controller as found in the > > + OLPC XO laptops. > > + > > + If your laptop doesn't have green ears, say "N" > I like this last comment :-) > > > + > > config DRM_LONTIUM_LT9611 > > tristate "Lontium LT9611 DSI/HDMI bridge" > > select SND_SOC_HDMI_CODEC if SND_SOC > > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile > > index 2b3aff104e466..21f72df3260db 100644 > > --- a/drivers/gpu/drm/bridge/Makefile > > +++ b/drivers/gpu/drm/bridge/Makefile > > @@ -2,6 +2,7 @@ > > obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o > > obj-$(CONFIG_DRM_CHRONTEL_CH7033) += chrontel-ch7033.o > > obj-$(CONFIG_DRM_DISPLAY_CONNECTOR) += display-connector.o > > +obj-$(CONFIG_DRM_HIMAX_HX8837) += himax-hx8837.o > > obj-$(CONFIG_DRM_LONTIUM_LT9611) += lontium-lt9611.o > > obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o > > obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o > > diff --git a/drivers/gpu/drm/bridge/himax-hx8837.c b/drivers/gpu/drm/bridge/himax-hx8837.c > > new file mode 100644 > > index 0000000000000..f472e16cc331d > > --- /dev/null > > +++ b/drivers/gpu/drm/bridge/himax-hx8837.c > > @@ -0,0 +1,330 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * HiMax HX8837 Display Controller Driver > > + * > > + * Datasheet: http://wiki.laptop.org/images/0/09/DCON_datasheet_HX8837-A.pdf > > + * > > + * Copyright (C) 2020 Lubomir Rintel > > + */ > > + > > +#include <drm/drm_atomic_helper.h> > > +#include <drm/drm_bridge.h> > > +#include <drm/drm_of.h> > > +#include <drm/drm_panel.h> > > +#include <drm/drm_print.h> > drm_print.h is no longer used and should be dropped. > > > +#include <drm/drm_probe_helper.h> > > + > > +#include <linux/gpio/consumer.h> > > +#include <linux/module.h> > > +#include <linux/regmap.h> > > + > > +#define bridge_to_hx8837_priv(x) \ > > + container_of(x, struct hx8837_priv, bridge) > > + > > +/* DCON registers */ > > +enum { > > + DCON_REG_ID = 0x00, > > + DCON_REG_MODE = 0x01, > > + DCON_REG_HRES = 0x02, > > + DCON_REG_HTOTAL = 0x03, > > + DCON_REG_HSYNC_WIDTH = 0x04, > > + DCON_REG_VRES = 0x05, > > + DCON_REG_VTOTAL = 0x06, > > + DCON_REG_VSYNC_WIDTH = 0x07, > > + DCON_REG_TIMEOUT = 0x08, > > + DCON_REG_SCAN_INT = 0x09, > > + DCON_REG_BRIGHT = 0x0a, > > + DCON_REG_MEM_OPT_A = 0x41, > > + DCON_REG_MEM_OPT_B = 0x42, > > +}; > > + > > +/* DCON_REG_MODE */ > > +enum { > > + MODE_PASSTHRU = BIT(0), > > + MODE_SLEEP = BIT(1), > > + MODE_SLEEP_AUTO = BIT(2), > > + MODE_BL_ENABLE = BIT(3), > > + MODE_BLANK = BIT(4), > > + MODE_CSWIZZLE = BIT(5), > > + MODE_COL_AA = BIT(6), > > + MODE_MONO_LUMA = BIT(7), > > + MODE_SCAN_INT = BIT(8), > > + MODE_CLOCKDIV = BIT(9), > > + MODE_DEBUG = BIT(14), > > + MODE_SELFTEST = BIT(15), > > +}; > > + > > +/* DCON_REG_BRIGHT */ > > +enum { > > + BRIGHT_MASK = GENMASK(7, 0), > > +}; > > + > > +struct hx8837_priv { > > + struct device *dev; > > + struct regmap *regmap; > > + struct gpio_desc *load_gpio; > > + > > + struct drm_bridge *panel_bridge; > > + struct drm_bridge bridge; > > +}; > > + > > +static int hx8837_bridge_attach(struct drm_bridge *bridge, > > + enum drm_bridge_attach_flags flags) > > +{ > > + struct hx8837_priv *priv = bridge_to_hx8837_priv(bridge); > > + > > + return drm_bridge_attach(bridge->encoder, priv->panel_bridge, > > + bridge, flags); > > +} > > + > > +static enum drm_mode_status hx8837_bridge_mode_valid( > > + struct drm_bridge *bridge, > > + const struct drm_display_info *info, > > + const struct drm_display_mode *mode) > > +{ > > + if (mode->hdisplay > 0xffff) > > + return MODE_BAD_HVALUE; > > + if (mode->htotal > 0xffff) > > + return MODE_BAD_HVALUE; > > + if (mode->hsync_start - mode->hdisplay > 0xff) > > + return MODE_HBLANK_WIDE; > > + if (mode->hsync_end - mode->hsync_start > 0xff) > > + return MODE_HSYNC_WIDE; > > + if (mode->vdisplay > 0xffff) > > + return MODE_BAD_VVALUE; > > + if (mode->vtotal > 0xffff) > > + return MODE_BAD_VVALUE; > > + if (mode->vsync_start - mode->vdisplay > 0xff) > > + return MODE_VBLANK_WIDE; > > + if (mode->vsync_end - mode->vsync_start > 0xff) > > + return MODE_VSYNC_WIDE; > > + > > + return MODE_OK; > > +} > > + > > +static void hx8837_bridge_disable(struct drm_bridge *bridge) > > +{ > > + struct hx8837_priv *priv = bridge_to_hx8837_priv(bridge); > > + int ret; > > + > > + ret = gpiod_direction_output(priv->load_gpio, 0); > > + if (ret) > > + dev_err(priv->dev, "error enabling the dcon load: %d\n", ret); > I assume this is disabling the dcon load - so text needs an update > > > + > > + ret = regmap_update_bits(priv->regmap, DCON_REG_MODE, > > + MODE_PASSTHRU | > > + MODE_SLEEP, > > + MODE_PASSTHRU | > > + MODE_SLEEP); > > + if (ret) > > + dev_err(priv->dev, "error disabling the dcon: %d\n", ret); > > +} > > + > > +static void hx8837_bridge_enable(struct drm_bridge *bridge) > > +{ > > + struct hx8837_priv *priv = bridge_to_hx8837_priv(bridge); > > + int ret; > > + > > + ret = regmap_update_bits(priv->regmap, DCON_REG_MODE, > > + MODE_PASSTHRU | > > + MODE_SLEEP | > > + MODE_SLEEP_AUTO | > > + MODE_BLANK | > > + MODE_SCAN_INT | > > + MODE_CLOCKDIV | > > + MODE_DEBUG | > > + MODE_SELFTEST, > > + MODE_PASSTHRU); > > + if (ret) > > + dev_err(priv->dev, "error enabling the dcon: %d\n", ret); > > + > > + ret = gpiod_direction_output(priv->load_gpio, 1); > > + if (ret) > > + dev_err(priv->dev, "error enabling the dcon load: %d\n", ret); > > +} > > + > > +static void hx8837_bridge_mode_set(struct drm_bridge *bridge, > > + const struct drm_display_mode *mode, > > + const struct drm_display_mode *adjusted_mode) > > +{ > > + struct hx8837_priv *priv = bridge_to_hx8837_priv(bridge); > > + > > + regmap_write(priv->regmap, DCON_REG_HRES, mode->hdisplay); > > + regmap_write(priv->regmap, DCON_REG_HTOTAL, mode->htotal); > > + regmap_write(priv->regmap, DCON_REG_HSYNC_WIDTH, > > + (mode->hsync_start - mode->hdisplay) << 8 | > > + (mode->hsync_end - mode->hsync_start)); > > + regmap_write(priv->regmap, DCON_REG_VRES, mode->vdisplay); > > + regmap_write(priv->regmap, DCON_REG_VTOTAL, mode->vtotal); > > + regmap_write(priv->regmap, DCON_REG_VSYNC_WIDTH, > > + (mode->vsync_start - mode->vdisplay) << 8 | > > + (mode->vsync_end - mode->vsync_start)); > > +} > > + > > +static const struct drm_bridge_funcs hx8837_bridge_funcs = { > > + .attach = hx8837_bridge_attach, > > + .mode_valid = hx8837_bridge_mode_valid, > > + .disable = hx8837_bridge_disable, > > + .enable = hx8837_bridge_enable, > > + .mode_set = hx8837_bridge_mode_set, > > +}; > > + > > +static int hx8837_bl_update_status(struct backlight_device *bl) > > +{ > > + struct hx8837_priv *priv = bl_get_data(bl); > > + unsigned int val; > > + int ret; > > + > > + ret = regmap_update_bits(priv->regmap, DCON_REG_BRIGHT, > > + BRIGHT_MASK, > > + backlight_get_brightness(bl)); > > + if (ret) { > > + dev_err(&bl->dev, "error setting the backlight: %d\n", ret); > > + return ret; > > + } > > + > > + if (backlight_get_brightness(bl)) > > + val = MODE_CSWIZZLE | MODE_COL_AA; > > + else > > + val = MODE_MONO_LUMA; > > + > > + ret = regmap_update_bits(priv->regmap, DCON_REG_MODE, > > + MODE_CSWIZZLE | > > + MODE_COL_AA | > > + MODE_MONO_LUMA, > > + val); > > + if (ret) { > > + dev_err(&bl->dev, "error setting color mode: %d\n", ret); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static const struct backlight_ops hx8837_bl_ops = { > > + .update_status = hx8837_bl_update_status, > > +}; > > + > > +static const struct regmap_config hx8837_regmap_config = { > > + .reg_bits = 8, > > + .val_bits = 16, > > + .max_register = 0x4c, > > + .val_format_endian = REGMAP_ENDIAN_LITTLE, > > +}; > > + > > +static int hx8837_probe(struct i2c_client *client, > > + const struct i2c_device_id *id) > > +{ > > + struct backlight_properties bl_props = { > > + .type = BACKLIGHT_RAW, > > + .max_brightness = BRIGHT_MASK, > > + }; > I have recently learned we shall also always set the scale. > This is missing in many drivers - but please do it here. > > > + struct device *dev = &client->dev; > > + struct backlight_device *bl; > > + struct hx8837_priv *priv; > > + struct drm_panel *panel; > > + unsigned int val; > > + int ret; > > + > > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > + dev_set_drvdata(dev, priv); > > + priv->dev = dev; > > + > > + priv->load_gpio = devm_gpiod_get(dev, "load", GPIOD_ASIS); > > + if (IS_ERR(priv->load_gpio)) > > + return PTR_ERR(priv->load_gpio); > > + > > + ret = drm_of_find_panel_or_bridge(dev->of_node, 1, -1, &panel, NULL); > > + if (ret) > > + return ret; > > + > > + if (panel->backlight) { > > + dev_err(dev, "the panel already has a backlight controller\n"); > > + return -ENODEV; > > + } > > + > > + priv->panel_bridge = devm_drm_panel_bridge_add(dev, panel); > > + if (IS_ERR(priv->panel_bridge)) > > + return PTR_ERR(priv->panel_bridge); > > + > > + priv->regmap = devm_regmap_init_i2c(client, &hx8837_regmap_config); > > + if (IS_ERR(priv->regmap)) { > > + dev_err(dev, "regmap init failed\n"); > regmap_init_i2c failed? > > > + return PTR_ERR(priv->regmap); > > + } > > + > > + ret = regmap_read(priv->regmap, DCON_REG_ID, &val); > > + if (ret < 0) { > > + dev_err(dev, "error reading the model id: %d\n", ret); > > + return ret; > > + } > > + if ((val & 0xff00) != 0xdc00) { > > + dev_err(dev, "the device is not a hx8837\n"); > > + return -ENODEV; > > + } > > + > > + ret = regmap_read(priv->regmap, DCON_REG_BRIGHT, &val); > > + if (ret < 0) { > > + dev_err(&bl->dev, "error getting the backlight: %d\n", ret); > > + return ret; > > + } > > + bl_props.brightness = val & 0xf; > > + > > + bl = devm_backlight_device_register(dev, dev_name(dev), dev, priv, > > + &hx8837_bl_ops, &bl_props); > > + if (IS_ERR(bl)) { > > + dev_err(dev, "failed to register backlight\n"); > > + return PTR_ERR(bl); > > + } > > + > > + panel->backlight = bl; > > + > > + INIT_LIST_HEAD(&priv->bridge.list); > > + priv->bridge.funcs = &hx8837_bridge_funcs; > > + priv->bridge.of_node = dev->of_node; > > + drm_bridge_add(&priv->bridge); > > + > > + dev_info(dev, "HiMax HX8837 Display Controller Driver\n"); > Debug - not needed > > + return 0; > > +} > > + > > +static int hx8837_remove(struct i2c_client *client) > > +{ > > + struct device *dev = &client->dev; > > + struct hx8837_priv *priv = dev_get_drvdata(dev); > > + > > + drm_bridge_remove(&priv->bridge); > > + > > + return 0; > > +} > > + > > +static const struct of_device_id hx8837_dt_ids[] = { > > + { .compatible = "himax,hx8837", }, > > + { /* sentinel */ } > > +}; > > +MODULE_DEVICE_TABLE(of, hx8837_dt_ids); > > + > > +static const struct i2c_device_id hx8837_ids[] = { > > + { "hx8837", 0 }, > > + { /* sentinel */ } > > +}; > > +MODULE_DEVICE_TABLE(i2c, hx8837_ids); > > + > > +static struct i2c_driver hx8837_driver = { > > + .probe = hx8837_probe, > > + .remove = hx8837_remove, > > + .driver = { > > + .name = "hx8837", > > + .of_match_table = of_match_ptr(hx8837_dt_ids), > > + }, > > + .id_table = hx8837_ids, > > +}; > > + > > +module_i2c_driver(hx8837_driver); > > + > > +MODULE_AUTHOR("Lubomir Rintel <lkundrak@v3.sk>"); > > +MODULE_DESCRIPTION("HiMax HX8837 Display Controller Driver"); > > +MODULE_LICENSE("GPL"); > > -- > > 2.28.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v6 2/2] drm/bridge: hx8837: add a Himax HX8837 display controller driver 2020-10-31 20:12 ` Lubomir Rintel @ 2020-10-31 20:56 ` Sam Ravnborg 0 siblings, 0 replies; 8+ messages in thread From: Sam Ravnborg @ 2020-10-31 20:56 UTC (permalink / raw) To: Lubomir Rintel Cc: Andrzej Hajda, Daniel Vetter, David Airlie, Rob Herring, Neil Armstrong, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, dri-devel, devicetree, linux-kernel, Rob Herring Hi Lubomir. > > > > > + select BACKLIGHT_CLASS_DEVICE > > Please use a depends - using select on a symbol with a prompt is always > > wrong. Yeah, I know you then need to enable backlight to see this > > driver. Sorry, but this is the best we can do now. > > Many other drivers can cope with depends here. > > This results in a dependency loop: > > drivers/video/fbdev/Kconfig:12:error: recursive dependency detected! > drivers/video/fbdev/Kconfig:12: symbol FB is selected by DRM_KMS_FB_HELPER > drivers/gpu/drm/Kconfig:80: symbol DRM_KMS_FB_HELPER depends on DRM_KMS_HELPER > drivers/gpu/drm/Kconfig:74: symbol DRM_KMS_HELPER is selected by DRM_HIMAX_HX8837 > drivers/gpu/drm/bridge/Kconfig:51: symbol DRM_HIMAX_HX8837 depends on BACKLIGHT_CLASS_DEVICE > drivers/video/backlight/Kconfig:143: symbol BACKLIGHT_CLASS_DEVICE is selected by FB_BACKLIGHT > drivers/video/fbdev/Kconfig:187: symbol FB_BACKLIGHT depends on FB > > Unfortunately I have no idea how to resolve it at the moment. > > I suppose I can look further into it if necessary. Or is it okay if I > leave it at select BACKLIGHT_CLASS_DEVICE for now? Sigh, leave it as a select then :-( The "sigh" is not directed at you but the mess this BACKLIGHT_CLASS_DEVICE is and the limitations of Kconfig. Sam ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-11-18 20:34 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-30 3:07 Lubomir Rintel 2020-10-30 3:07 ` [PATCH v6 1/2] dt-bindings: display: himax,hx8837: Add Himax HX8837 bindings Lubomir Rintel 2020-11-01 16:39 ` Laurent Pinchart 2020-11-18 20:34 ` Lubomir Rintel 2020-10-30 3:08 ` [PATCH v6 2/2] drm/bridge: hx8837: add a Himax HX8837 display controller driver Lubomir Rintel 2020-10-31 8:01 ` Sam Ravnborg 2020-10-31 20:12 ` Lubomir Rintel 2020-10-31 20:56 ` Sam Ravnborg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).