* [PATCH v2 0/4] Add it66121 driver
@ 2020-03-11 12:51 Phong LE
2020-03-11 12:51 ` [PATCH v2 1/4] dt-bindings: add ITE vendor Phong LE
` (3 more replies)
0 siblings, 4 replies; 17+ messages in thread
From: Phong LE @ 2020-03-11 12:51 UTC (permalink / raw)
To: narmstrong, airlied, daniel, robh+dt, mark.rutland, a.hajda
Cc: Laurent.pinchart, jonas, jernej.skrabec, sam, mripard,
heiko.stuebner, linus.walleij, stephan, icenowy, broonie,
mchehab+samsung, davem, gregkh, Jonathan.Cameron,
andriy.shevchenko, dri-devel, devicetree, linux-kernel
The IT66121 is a high-performance and low-power single channel HDMI
transmitter, fully compliant with HDMI 1.3a, HDCP 1.2 and backward
compatible to DVI 1.0 specifications.
It supports pixel rates from 25MHz to 165MHz.
This series contains document bindings, add vendor prefix, Kconfig to
enable or not.
For now, the driver handles only RGB without color conversion.
Audio, CEC and HDCP are not implemented yet.
Phong LE (4):
dt-bindings: add ITE vendor
dt-bindings: display: bridge: add it66121 bindings
drm: bridge: add it66121 driver
MAINTAINERS: add it66121 HDMI bridge driver entry
.../bindings/display/bridge/ite,it66121.yaml | 98 ++
.../devicetree/bindings/vendor-prefixes.yaml | 2 +
MAINTAINERS | 8 +
drivers/gpu/drm/bridge/Kconfig | 8 +
drivers/gpu/drm/bridge/Makefile | 1 +
drivers/gpu/drm/bridge/ite-it66121.c | 997 ++++++++++++++++++
6 files changed, 1114 insertions(+)
create mode 100644 Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
create mode 100644 drivers/gpu/drm/bridge/ite-it66121.c
--
2.17.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 1/4] dt-bindings: add ITE vendor
2020-03-11 12:51 [PATCH v2 0/4] Add it66121 driver Phong LE
@ 2020-03-11 12:51 ` Phong LE
2020-03-23 22:04 ` Rob Herring
2020-03-11 12:51 ` [PATCH v2 2/4] dt-bindings: display: bridge: add it66121 bindings Phong LE
` (2 subsequent siblings)
3 siblings, 1 reply; 17+ messages in thread
From: Phong LE @ 2020-03-11 12:51 UTC (permalink / raw)
To: narmstrong, airlied, daniel, robh+dt, mark.rutland, a.hajda
Cc: Laurent.pinchart, jonas, jernej.skrabec, sam, mripard,
heiko.stuebner, linus.walleij, stephan, icenowy, broonie,
mchehab+samsung, davem, gregkh, Jonathan.Cameron,
andriy.shevchenko, dri-devel, devicetree, linux-kernel
Add ITE Tech Inc. prefix "ite" in vendor-prefixes. More information on:
http://www.ite.com.tw/
Signed-off-by: Phong LE <ple@baylibre.com>
---
Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index a2da166df1bc..c2fb444e1900 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -475,6 +475,8 @@ patternProperties:
description: Intersil
"^issi,.*":
description: Integrated Silicon Solutions Inc.
+ "^ite,.*":
+ description: ITE Tech Inc.
"^itead,.*":
description: ITEAD Intelligent Systems Co.Ltd
"^iwave,.*":
--
2.17.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 2/4] dt-bindings: display: bridge: add it66121 bindings
2020-03-11 12:51 [PATCH v2 0/4] Add it66121 driver Phong LE
2020-03-11 12:51 ` [PATCH v2 1/4] dt-bindings: add ITE vendor Phong LE
@ 2020-03-11 12:51 ` Phong LE
2020-03-13 13:40 ` Laurent Pinchart
2020-03-23 22:09 ` Rob Herring
2020-03-11 12:51 ` [PATCH v2 3/4] drm: bridge: add it66121 driver Phong LE
2020-03-11 12:51 ` [PATCH v2 4/4] MAINTAINERS: add it66121 HDMI bridge driver entry Phong LE
3 siblings, 2 replies; 17+ messages in thread
From: Phong LE @ 2020-03-11 12:51 UTC (permalink / raw)
To: narmstrong, airlied, daniel, robh+dt, mark.rutland, a.hajda
Cc: Laurent.pinchart, jonas, jernej.skrabec, sam, mripard,
heiko.stuebner, linus.walleij, stephan, icenowy, broonie,
mchehab+samsung, davem, gregkh, Jonathan.Cameron,
andriy.shevchenko, dri-devel, devicetree, linux-kernel
Add the ITE bridge HDMI it66121 bindings.
Signed-off-by: Phong LE <ple@baylibre.com>
---
.../bindings/display/bridge/ite,it66121.yaml | 98 +++++++++++++++++++
1 file changed, 98 insertions(+)
create mode 100644 Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
diff --git a/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
new file mode 100644
index 000000000000..1717e880d130
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
@@ -0,0 +1,98 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/bridge/ite,it66121.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ITE it66121 HDMI bridge Device Tree Bindings
+
+maintainers:
+ - Phong LE <ple@baylibre.com>
+ - Neil Armstrong <narmstrong@baylibre.com>
+
+description: |
+ The IT66121 is a high-performance and low-power single channel HDMI
+ transmitter, fully compliant with HDMI 1.3a, HDCP 1.2 and backward compatible
+ to DVI 1.0 specifications.
+
+properties:
+ compatible:
+ const: ite,it66121
+
+ reg:
+ maxItems: 1
+ description: base I2C address of the device
+
+ reset-gpios:
+ maxItems: 1
+ description: GPIO connected to active low reset
+
+ vrf12-supply:
+ maxItems: 1
+ description: Regulator for 1.2V analog core power.
+
+ vcn33-supply:
+ maxItems: 1
+ description: Regulator for 3.3V digital core power.
+
+ vcn18-supply:
+ maxItems: 1
+ description: Regulator for 1.8V IO core power.
+
+ interrupts:
+ maxItems: 1
+
+ pclk-dual-edge:
+ maxItems: 1
+ description: enable pclk dual edge mode.
+
+ port:
+ type: object
+
+ properties:
+ endpoint:
+ type: object
+ description: |
+ Input endpoints of the bridge.
+
+ required:
+ - endpoint
+
+required:
+ - compatible
+ - reg
+ - reset-gpios
+ - vrf12-supply
+ - vcn33-supply
+ - vcn18-supply
+ - interrupts
+ - port
+
+additionalProperties: false
+
+examples:
+ - |
+ i2c6 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ it66121hdmitx: it66121hdmitx@4c {
+ compatible = "ite,it66121";
+ pinctrl-names = "default";
+ pinctrl-0 = <&ite_pins_default>;
+ vcn33-supply = <&mt6358_vcn33_wifi_reg>;
+ vcn18-supply = <&mt6358_vcn18_reg>;
+ vrf12-supply = <&mt6358_vrf12_reg>;
+ reset-gpios = <&pio 160 1 /* GPIO_ACTIVE_LOW */>;
+ interrupt-parent = <&pio>;
+ interrupts = <4 8 /* IRQ_TYPE_LEVEL_LOW */>;
+ reg = <0x4c>;
+ pclk-dual-edge;
+
+ port {
+ it66121_in: endpoint {
+ remote-endpoint = <&display_out>;
+ };
+ };
+ };
+ };
--
2.17.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 3/4] drm: bridge: add it66121 driver
2020-03-11 12:51 [PATCH v2 0/4] Add it66121 driver Phong LE
2020-03-11 12:51 ` [PATCH v2 1/4] dt-bindings: add ITE vendor Phong LE
2020-03-11 12:51 ` [PATCH v2 2/4] dt-bindings: display: bridge: add it66121 bindings Phong LE
@ 2020-03-11 12:51 ` Phong LE
2020-03-11 13:55 ` Andy Shevchenko
` (2 more replies)
2020-03-11 12:51 ` [PATCH v2 4/4] MAINTAINERS: add it66121 HDMI bridge driver entry Phong LE
3 siblings, 3 replies; 17+ messages in thread
From: Phong LE @ 2020-03-11 12:51 UTC (permalink / raw)
To: narmstrong, airlied, daniel, robh+dt, mark.rutland, a.hajda
Cc: Laurent.pinchart, jonas, jernej.skrabec, sam, mripard,
heiko.stuebner, linus.walleij, stephan, icenowy, broonie,
mchehab+samsung, davem, gregkh, Jonathan.Cameron,
andriy.shevchenko, dri-devel, devicetree, linux-kernel
This commit is a simple driver for bridge HMDI it66121.
The input format is RBG and there is no color conversion.
Audio, HDCP and CEC are not supported yet.
Signed-off-by: Phong LE <ple@baylibre.com>
---
drivers/gpu/drm/bridge/Kconfig | 8 +
drivers/gpu/drm/bridge/Makefile | 1 +
drivers/gpu/drm/bridge/ite-it66121.c | 997 +++++++++++++++++++++++++++
3 files changed, 1006 insertions(+)
create mode 100644 drivers/gpu/drm/bridge/ite-it66121.c
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index aaed2347ace9..ae57b83f47c8 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -38,6 +38,14 @@ config DRM_DISPLAY_CONNECTOR
on ARM-based platforms. Saying Y here when this driver is not needed
will not cause any issue.
+config DRM_ITE_IT66121
+ tristate "ITE IT66121 HDMI bridge"
+ depends on OF
+ select DRM_KMS_HELPER
+ select REGMAP_I2C
+ help
+ Support for ITE IT66121 HDMI bridge.
+
config DRM_LVDS_CODEC
tristate "Transparent LVDS encoders and decoders support"
depends on OF
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 6fb062b5b0f0..9fe80e820f64 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -1,6 +1,7 @@
# SPDX-License-Identifier: GPL-2.0
obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
obj-$(CONFIG_DRM_DISPLAY_CONNECTOR) += display-connector.o
+obj-$(CONFIG_DRM_ITE_IT66121) += ite-it66121.o
obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o
obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
new file mode 100644
index 000000000000..7e1a90319a6a
--- /dev/null
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -0,0 +1,997 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2020 BayLibre, SAS
+ * Author: Phong LE <ple@baylibre.com>
+ * Copyright (C) 2018-2019, Artem Mygaiev
+ * Copyright (C) 2017, Fresco Logic, Incorporated.
+ *
+ */
+
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_bridge.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_edid.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_print.h>
+#include <drm/drm_probe_helper.h>
+
+#define IT66121_MASTER_SEL_REG 0x10
+#define IT66121_MASTER_SEL_HOST BIT(0)
+
+#define IT66121_AFE_DRV_REG 0x61
+#define IT66121_AFE_DRV_RST BIT(4)
+#define IT66121_AFE_DRV_PWD BIT(5)
+
+#define IT66121_INPUT_MODE_REG 0x70
+#define IT66121_INPUT_MODE_RGB (0 << 6)
+#define IT66121_INPUT_MODE_YUV422 BIT(6)
+#define IT66121_INPUT_MODE_YUV444 (2 << 6)
+#define IT66121_INPUT_MODE_CCIR656 BIT(4)
+#define IT66121_INPUT_MODE_SYNCEMB BIT(3)
+#define IT66121_INPUT_MODE_DDR BIT(2)
+
+#define IT66121_INPUT_CSC_REG 0x72
+#define IT66121_INPUT_CSC_ENDITHER BIT(7)
+#define IT66121_INPUT_CSC_ENUDFILTER BIT(6)
+#define IT66121_INPUT_CSC_DNFREE_GO BIT(5)
+#define IT66121_INPUT_CSC_RGB_TO_YUV 0x02
+#define IT66121_INPUT_CSC_YUV_TO_RGB 0x03
+#define IT66121_INPUT_CSC_NO_CONV 0x00
+
+#define IT66121_AFE_XP_REG 0x62
+#define IT66121_AFE_XP_GAINBIT BIT(7)
+#define IT66121_AFE_XP_PWDPLL BIT(6)
+#define IT66121_AFE_XP_ENI BIT(5)
+#define IT66121_AFE_XP_ENO BIT(4)
+#define IT66121_AFE_XP_RESETB BIT(3)
+#define IT66121_AFE_XP_PWDI BIT(2)
+
+#define IT66121_AFE_IP_REG 0x64
+#define IT66121_AFE_IP_GAINBIT BIT(7)
+#define IT66121_AFE_IP_PWDPLL BIT(6)
+#define IT66121_AFE_IP_CKSEL_05 (0 << 4)
+#define IT66121_AFE_IP_CKSEL_1 BIT(4)
+#define IT66121_AFE_IP_CKSEL_2 (2 << 4)
+#define IT66121_AFE_IP_CKSEL_2OR4 (3 << 4)
+#define IT66121_AFE_IP_ER0 BIT(3)
+#define IT66121_AFE_IP_RESETB BIT(2)
+#define IT66121_AFE_IP_ENC BIT(1)
+#define IT66121_AFE_IP_EC1 BIT(0)
+
+#define IT66121_AFE_XP_EC1_REG 0x68
+#define IT66121_AFE_XP_EC1_LOWCLK BIT(4)
+
+#define IT66121_SW_RST_REG 0x04
+#define IT66121_SW_RST_REF BIT(5)
+#define IT66121_SW_RST_AREF BIT(4)
+#define IT66121_SW_RST_VID BIT(3)
+#define IT66121_SW_RST_AUD BIT(2)
+#define IT66121_SW_RST_HDCP BIT(0)
+
+#define IT66121_DDC_COMMAND_REG 0x15
+#define IT66121_DDC_COMMAND_BURST_READ 0x0
+#define IT66121_DDC_COMMAND_EDID_READ 0x3
+#define IT66121_DDC_COMMAND_FIFO_CLR 0x9
+#define IT66121_DDC_COMMAND_SCL_PULSE 0xA
+#define IT66121_DDC_COMMAND_ABORT 0xF
+
+#define IT66121_HDCP_REG 0x20
+#define IT66121_HDCP_CPDESIRED BIT(0)
+#define IT66121_HDCP_EN1P1FEAT BIT(1)
+
+#define IT66121_INT_STATUS1_REG 0x06
+#define IT66121_INT_STATUS1_AUD_OVF BIT(7)
+#define IT66121_INT_STATUS1_DDC_NOACK BIT(5)
+#define IT66121_INT_STATUS1_DDC_FIFOERR BIT(4)
+#define IT66121_INT_STATUS1_DDC_BUSHANG BIT(2)
+#define IT66121_INT_STATUS1_RX_SENS_STATUS BIT(1)
+#define IT66121_INT_STATUS1_HPD_STATUS BIT(0)
+
+#define IT66121_DDC_HEADER_REG 0x11
+#define IT66121_DDC_HEADER_HDCP 0x74
+#define IT66121_DDC_HEADER_EDID 0xA0
+
+#define IT66121_DDC_OFFSET_REG 0x12
+#define IT66121_DDC_BYTE_REG 0x13
+#define IT66121_DDC_SEGMENT_REG 0x14
+#define IT66121_DDC_RD_FIFO_REG 0x17
+
+#define IT66121_CLK_BANK_REG 0x0F
+#define IT66121_CLK_BANK_PWROFF_RCLK BIT(6)
+#define IT66121_CLK_BANK_PWROFF_ACLK BIT(5)
+#define IT66121_CLK_BANK_PWROFF_TXCLK BIT(4)
+#define IT66121_CLK_BANK_PWROFF_CRCLK BIT(3)
+#define IT66121_CLK_BANK_0 0
+#define IT66121_CLK_BANK_1 1
+
+#define IT66121_INT_REG 0x05
+#define IT66121_INT_ACTIVE_HIGH BIT(7)
+#define IT66121_INT_OPEN_DRAIN BIT(6)
+#define IT66121_INT_TX_CLK_OFF BIT(0)
+
+#define IT66121_INT_MASK1_REG 0x09
+#define IT66121_INT_MASK1_AUD_OVF BIT(7)
+#define IT66121_INT_MASK1_DDC_NOACK BIT(5)
+#define IT66121_INT_MASK1_DDC_FIFOERR BIT(4)
+#define IT66121_INT_MASK1_DDC_BUSHANG BIT(2)
+#define IT66121_INT_MASK1_RX_SENS BIT(1)
+#define IT66121_INT_MASK1_HPD BIT(0)
+
+#define IT66121_INT_CLR1_REG 0x0C
+#define IT66121_INT_CLR1_PKTACP BIT(7)
+#define IT66121_INT_CLR1_PKTNULL BIT(6)
+#define IT66121_INT_CLR1_PKTGEN BIT(5)
+#define IT66121_INT_CLR1_KSVLISTCHK BIT(4)
+#define IT66121_INT_CLR1_AUTHDONE BIT(3)
+#define IT66121_INT_CLR1_AUTHFAIL BIT(2)
+#define IT66121_INT_CLR1_RX_SENS BIT(1)
+#define IT66121_INT_CLR1_HPD BIT(0)
+
+#define IT66121_AV_MUTE_REG 0xC1
+#define IT66121_AV_MUTE_ON BIT(0)
+#define IT66121_AV_MUTE_BLUESCR BIT(1)
+
+#define IT66121_PKT_GEN_CTRL_REG 0xC6
+#define IT66121_PKT_GEN_CTRL_ON BIT(0)
+#define IT66121_PKT_GEN_CTRL_RPT BIT(1)
+
+#define IT66121_AVIINFO_DB1_REG 0x158
+#define IT66121_AVIINFO_DB2_REG 0x159
+#define IT66121_AVIINFO_DB3_REG 0x15A
+#define IT66121_AVIINFO_DB4_REG 0x15B
+#define IT66121_AVIINFO_DB5_REG 0x15C
+#define IT66121_AVIINFO_CSUM_REG 0x15D
+#define IT66121_AVIINFO_DB6_REG 0x15E
+#define IT66121_AVIINFO_DB7_REG 0x15F
+#define IT66121_AVIINFO_DB8_REG 0x160
+#define IT66121_AVIINFO_DB9_REG 0x161
+#define IT66121_AVIINFO_DB10_REG 0x162
+#define IT66121_AVIINFO_DB11_REG 0x163
+#define IT66121_AVIINFO_DB12_REG 0x164
+#define IT66121_AVIINFO_DB13_REG 0x165
+
+#define IT66121_AVI_INFO_PKT_REG 0xCD
+#define IT66121_AVI_INFO_PKT_ON BIT(0)
+#define IT66121_AVI_INFO_PKT_RPT BIT(1)
+
+#define IT66121_HDMI_MODE_REG 0xC0
+#define IT66121_HDMI_MODE_HDMI BIT(0)
+
+#define IT66121_SYS_STATUS_REG 0x0E
+#define IT66121_SYS_STATUS_ACTIVE_IRQ BIT(7)
+#define IT66121_SYS_STATUS_HPDETECT BIT(6)
+#define IT66121_SYS_STATUS_SENDECTECT BIT(5)
+#define IT66121_SYS_STATUS_VID_STABLE BIT(4)
+#define IT66121_SYS_STATUS_AUD_CTS_CLR BIT(1)
+#define IT66121_SYS_STATUS_CLEAR_IRQ BIT(0)
+
+#define IT66121_DDC_STATUS_REG 0x16
+#define IT66121_DDC_STATUS_TX_DONE BIT(7)
+#define IT66121_DDC_STATUS_ACTIVE BIT(6)
+#define IT66121_DDC_STATUS_NOACK BIT(5)
+#define IT66121_DDC_STATUS_WAIT_BUS BIT(4)
+#define IT66121_DDC_STATUS_ARBI_LOSE BIT(3)
+#define IT66121_DDC_STATUS_FIFO_FULL BIT(2)
+#define IT66121_DDC_STATUS_FIFO_EMPTY BIT(1)
+#define IT66121_DDC_STATUS_FIFO_VALID BIT(0)
+
+#define IT66121_VENDOR_ID0 0x54
+#define IT66121_VENDOR_ID1 0x49
+#define IT66121_DEVICE_ID0 0x12
+#define IT66121_DEVICE_ID1 0x06
+#define IT66121_DEVICE_MASK 0x0F
+#define IT66121_EDID_SLEEP 20000
+#define IT66121_EDID_TIMEOUT 200000
+#define IT66121_EDID_FIFO_SIZE 32
+#define IT66121_AFE_CLK_HIGH 80000
+
+struct it66121_conf {
+ unsigned int input_mode_reg;
+ unsigned int input_conversion_reg;
+};
+
+struct it66121_ctx {
+ struct regmap *regmap;
+ struct drm_bridge bridge;
+ struct drm_connector connector;
+ struct device *dev;
+ struct gpio_desc *gpio_reset;
+ struct i2c_client *client;
+ struct regulator_bulk_data supplies[3];
+ bool dual_edge;
+ const struct it66121_conf *conf;
+ struct mutex lock; /* Protects fields below and device registers */
+ struct edid *edid;
+ struct hdmi_avi_infoframe hdmi_avi_infoframe;
+};
+
+static const struct regmap_range_cfg it66121_regmap_banks[] = {
+ {
+ .name = "it66121",
+ .range_min = 0x00,
+ .range_max = 0x1FF,
+ .selector_reg = IT66121_CLK_BANK_REG,
+ .selector_mask = 0x1,
+ .selector_shift = 0,
+ .window_start = 0x00,
+ .window_len = 0x130,
+ },
+};
+
+static const struct regmap_config it66121_regmap_config = {
+ .val_bits = 8,
+ .reg_bits = 8,
+ .max_register = 0x1FF,
+ .ranges = it66121_regmap_banks,
+ .num_ranges = ARRAY_SIZE(it66121_regmap_banks),
+};
+
+static const struct it66121_conf it66121_conf_simple = {
+ .input_mode_reg = IT66121_INPUT_MODE_RGB | IT66121_INPUT_MODE_DDR,
+ .input_conversion_reg = IT66121_INPUT_CSC_NO_CONV,
+};
+
+static void it66121_hw_reset(struct it66121_ctx *ctx)
+{
+ gpiod_set_value(ctx->gpio_reset, 1);
+ msleep(20);
+ gpiod_set_value(ctx->gpio_reset, 0);
+}
+
+static int ite66121_power_on(struct it66121_ctx *ctx)
+{
+ return regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
+}
+
+static int ite66121_power_off(struct it66121_ctx *ctx)
+{
+ return regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
+}
+
+static int it66121_preamble_ddc(struct it66121_ctx *ctx)
+{
+ return regmap_write(ctx->regmap, IT66121_MASTER_SEL_REG,
+ IT66121_MASTER_SEL_HOST);
+}
+
+static int it66121_fire_afe(struct it66121_ctx *ctx)
+{
+ return regmap_write(ctx->regmap, IT66121_AFE_DRV_REG, 0);
+}
+
+static int it66121_configure_input(struct it66121_ctx *ctx)
+{
+ int ret;
+
+ ret = regmap_write(ctx->regmap, IT66121_INPUT_MODE_REG,
+ ctx->conf->input_mode_reg);
+ if (ret)
+ return ret;
+
+ return regmap_write(ctx->regmap, IT66121_INPUT_CSC_REG,
+ ctx->conf->input_conversion_reg);
+}
+
+/**
+ * it66121_configure_afe() - Configure the analog front end
+ * @ctx: it66121_ctx object
+ *
+ * RETURNS:
+ * zero if success, a negative error code otherwise.
+ */
+static int it66121_configure_afe(struct it66121_ctx *ctx,
+ const struct drm_display_mode *mode)
+{
+ int ret;
+
+ ret = regmap_write(ctx->regmap, IT66121_AFE_DRV_REG,
+ IT66121_AFE_DRV_RST);
+ if (ret)
+ return ret;
+
+ if (mode->clock > IT66121_AFE_CLK_HIGH) {
+ ret = regmap_write_bits(ctx->regmap, IT66121_AFE_XP_REG,
+ IT66121_AFE_XP_GAINBIT |
+ IT66121_AFE_XP_ENO,
+ IT66121_AFE_XP_GAINBIT);
+ if (ret)
+ return ret;
+
+ ret = regmap_write_bits(ctx->regmap, IT66121_AFE_IP_REG,
+ IT66121_AFE_IP_GAINBIT |
+ IT66121_AFE_IP_ER0 |
+ IT66121_AFE_IP_EC1,
+ IT66121_AFE_IP_GAINBIT);
+ if (ret)
+ return ret;
+
+ ret = regmap_write_bits(ctx->regmap, IT66121_AFE_XP_EC1_REG,
+ IT66121_AFE_XP_EC1_LOWCLK, 0x80);
+ if (ret)
+ return ret;
+ } else {
+ ret = regmap_write_bits(ctx->regmap, IT66121_AFE_XP_REG,
+ IT66121_AFE_XP_GAINBIT |
+ IT66121_AFE_XP_ENO,
+ IT66121_AFE_XP_ENO);
+ if (ret)
+ return ret;
+
+ ret = regmap_write_bits(ctx->regmap, IT66121_AFE_IP_REG,
+ IT66121_AFE_IP_GAINBIT |
+ IT66121_AFE_IP_ER0 |
+ IT66121_AFE_IP_EC1, IT66121_AFE_IP_ER0 |
+ IT66121_AFE_IP_EC1);
+ if (ret)
+ return ret;
+
+ ret = regmap_write_bits(ctx->regmap, IT66121_AFE_XP_EC1_REG,
+ IT66121_AFE_XP_EC1_LOWCLK,
+ IT66121_AFE_XP_EC1_LOWCLK);
+ if (ret)
+ return ret;
+ }
+
+ /* Clear reset flags */
+ ret = regmap_write_bits(ctx->regmap, IT66121_SW_RST_REG,
+ IT66121_SW_RST_REF | IT66121_SW_RST_VID,
+ ~(IT66121_SW_RST_REF | IT66121_SW_RST_VID) &
+ 0xFF);
+ if (ret)
+ return ret;
+
+ return it66121_fire_afe(ctx);
+}
+
+static inline int it66121_wait_ddc_ready(struct it66121_ctx *ctx)
+{
+ int ret, val;
+
+ ret = regmap_read_poll_timeout(ctx->regmap, IT66121_DDC_STATUS_REG,
+ val, true,
+ IT66121_EDID_SLEEP,
+ IT66121_EDID_TIMEOUT);
+ if (ret)
+ return ret;
+
+ if (val & (IT66121_DDC_STATUS_NOACK | IT66121_DDC_STATUS_WAIT_BUS |
+ IT66121_DDC_STATUS_ARBI_LOSE))
+ return -EAGAIN;
+
+ return 0;
+}
+
+static int it66121_clear_ddc_fifo(struct it66121_ctx *ctx)
+{
+ int ret;
+
+ ret = it66121_preamble_ddc(ctx);
+ if (ret)
+ return ret;
+
+ return regmap_write(ctx->regmap, IT66121_DDC_COMMAND_REG,
+ IT66121_DDC_COMMAND_FIFO_CLR);
+}
+
+static int it66121_abort_ddc_ops(struct it66121_ctx *ctx)
+{
+ int ret;
+ unsigned int swreset, cpdesire;
+
+ ret = regmap_read(ctx->regmap, IT66121_SW_RST_REG, &swreset);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(ctx->regmap, IT66121_HDCP_REG, &cpdesire);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(ctx->regmap, IT66121_HDCP_REG,
+ cpdesire & (~IT66121_HDCP_CPDESIRED & 0xFF));
+ if (ret)
+ return ret;
+
+ ret = regmap_write(ctx->regmap, IT66121_SW_RST_REG,
+ swreset | IT66121_SW_RST_HDCP);
+ if (ret)
+ return ret;
+
+ ret = it66121_preamble_ddc(ctx);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(ctx->regmap, IT66121_DDC_COMMAND_REG,
+ IT66121_DDC_COMMAND_ABORT);
+ if (ret)
+ return ret;
+
+ return it66121_wait_ddc_ready(ctx);
+}
+
+static int it66121_get_edid_block(void *context, u8 *buf,
+ unsigned int block, size_t len)
+{
+ struct it66121_ctx *ctx = context;
+ unsigned int val;
+ int remain = len;
+ int offset = 0;
+ int ret, cnt;
+
+ offset = (block % 2) * len;
+ block = block / 2;
+
+ ret = regmap_read(ctx->regmap, IT66121_INT_STATUS1_REG, &val);
+ if (ret)
+ return ret;
+
+ if (val & IT66121_INT_STATUS1_DDC_BUSHANG) {
+ ret = it66121_abort_ddc_ops(ctx);
+ if (ret)
+ return ret;
+ }
+
+ ret = it66121_clear_ddc_fifo(ctx);
+ if (ret)
+ return ret;
+
+ while (remain > 0) {
+ cnt = (remain > IT66121_EDID_FIFO_SIZE) ?
+ IT66121_EDID_FIFO_SIZE : remain;
+ ret = it66121_preamble_ddc(ctx);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(ctx->regmap, IT66121_DDC_COMMAND_REG,
+ IT66121_DDC_COMMAND_FIFO_CLR);
+ if (ret)
+ return ret;
+
+ ret = it66121_wait_ddc_ready(ctx);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(ctx->regmap, IT66121_INT_STATUS1_REG, &val);
+ if (ret)
+ return ret;
+
+ if (val & IT66121_INT_STATUS1_DDC_BUSHANG) {
+ ret = it66121_abort_ddc_ops(ctx);
+ if (ret)
+ return ret;
+ }
+
+ ret = it66121_preamble_ddc(ctx);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(ctx->regmap, IT66121_DDC_HEADER_REG,
+ IT66121_DDC_HEADER_EDID);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(ctx->regmap, IT66121_DDC_OFFSET_REG, offset);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(ctx->regmap, IT66121_DDC_BYTE_REG, cnt);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(ctx->regmap, IT66121_DDC_SEGMENT_REG, block);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(ctx->regmap, IT66121_DDC_COMMAND_REG,
+ IT66121_DDC_COMMAND_EDID_READ);
+ if (ret)
+ return ret;
+
+ offset += cnt;
+ remain -= cnt;
+ msleep(20);
+
+ ret = it66121_wait_ddc_ready(ctx);
+ if (ret)
+ return ret;
+
+ do {
+ ret = regmap_read(ctx->regmap,
+ IT66121_DDC_RD_FIFO_REG, &val);
+ if (ret)
+ return ret;
+ *(buf++) = val;
+ cnt--;
+ } while (cnt > 0);
+ }
+
+ return 0;
+}
+
+static int it66121_connector_get_modes(struct drm_connector *connector)
+{
+ int ret, num_modes = 0;
+ struct it66121_ctx *ctx = container_of(connector, struct it66121_ctx,
+ connector);
+
+ if (ctx->edid)
+ return drm_add_edid_modes(connector, ctx->edid);
+
+ mutex_lock(&ctx->lock);
+
+ ctx->edid = drm_do_get_edid(connector, it66121_get_edid_block, ctx);
+ if (!ctx->edid) {
+ DRM_ERROR("Failed to read EDID\n");
+ goto unlock;
+ }
+
+ ret = drm_connector_update_edid_property(connector,
+ ctx->edid);
+ if (ret) {
+ DRM_ERROR("Failed to update EDID property: %d\n", ret);
+ goto unlock;
+ }
+
+ num_modes = drm_add_edid_modes(connector, ctx->edid);
+
+unlock:
+ mutex_unlock(&ctx->lock);
+
+ return num_modes;
+}
+
+static bool it66121_is_hpd_detect(struct it66121_ctx *ctx)
+{
+ int val;
+
+ if (regmap_read(ctx->regmap, IT66121_SYS_STATUS_REG, &val))
+ return false;
+
+ return (val & IT66121_SYS_STATUS_HPDETECT);
+}
+
+static int it66121_connector_detect_ctx(struct drm_connector *connector,
+ struct drm_modeset_acquire_ctx *c,
+ bool force)
+{
+ struct it66121_ctx *ctx = container_of(connector, struct it66121_ctx,
+ connector);
+
+ return (it66121_is_hpd_detect(ctx)) ?
+ connector_status_connected : connector_status_disconnected;
+}
+
+static enum drm_mode_status
+it66121_connector_mode_valid(struct drm_connector *connector,
+ struct drm_display_mode *mode)
+{
+ unsigned long max_clock;
+ struct it66121_ctx *ctx = container_of(connector, struct it66121_ctx,
+ connector);
+
+ max_clock = ctx->dual_edge ? 74250 : 148500;
+
+ if (mode->clock > max_clock)
+ return MODE_CLOCK_HIGH;
+
+ if (mode->clock < 25000)
+ return MODE_CLOCK_LOW;
+
+ return MODE_OK;
+}
+
+static struct drm_connector_helper_funcs it66121_connector_helper_funcs = {
+ .get_modes = it66121_connector_get_modes,
+ .detect_ctx = it66121_connector_detect_ctx,
+ .mode_valid = it66121_connector_mode_valid,
+};
+
+static const struct drm_connector_funcs it66121_connector_funcs = {
+ .reset = drm_atomic_helper_connector_reset,
+ .fill_modes = drm_helper_probe_single_connector_modes,
+ .destroy = drm_connector_cleanup,
+ .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static int it66121_bridge_attach(struct drm_bridge *bridge,
+ enum drm_bridge_attach_flags flags)
+{
+ int ret;
+ struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx,
+ bridge);
+
+ if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
+ DRM_ERROR("Fix bridge driver to make connector optional!");
+ return -EINVAL;
+ }
+
+ if (!bridge->encoder) {
+ DRM_ERROR("Parent encoder object not found");
+ return -ENODEV;
+ }
+
+ ret = regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
+ IT66121_CLK_BANK_PWROFF_RCLK, 0);
+ if (ret)
+ return ret;
+
+ ret = regmap_write_bits(ctx->regmap, IT66121_INT_REG,
+ IT66121_INT_TX_CLK_OFF, 0);
+ if (ret)
+ return ret;
+
+ ret = regmap_write_bits(ctx->regmap, IT66121_AFE_DRV_REG,
+ IT66121_AFE_DRV_PWD, 0);
+ if (ret)
+ return ret;
+
+ ret = regmap_write_bits(ctx->regmap, IT66121_AFE_XP_REG,
+ IT66121_AFE_XP_PWDI | IT66121_AFE_XP_PWDPLL, 0);
+ if (ret)
+ return ret;
+
+ ret = regmap_write_bits(ctx->regmap, IT66121_AFE_IP_REG,
+ IT66121_AFE_IP_PWDPLL, 0);
+ if (ret)
+ return ret;
+
+ ret = regmap_write_bits(ctx->regmap, IT66121_AFE_DRV_REG,
+ IT66121_AFE_DRV_RST, 0);
+ if (ret)
+ return ret;
+
+ ret = regmap_write_bits(ctx->regmap, IT66121_AFE_XP_REG,
+ IT66121_AFE_XP_RESETB, IT66121_AFE_XP_RESETB);
+ if (ret)
+ return ret;
+
+ ret = regmap_write_bits(ctx->regmap, IT66121_AFE_IP_REG,
+ IT66121_AFE_IP_RESETB, IT66121_AFE_IP_RESETB);
+ if (ret)
+ return ret;
+
+ ret = regmap_write_bits(ctx->regmap, IT66121_SW_RST_REG,
+ IT66121_SW_RST_REF,
+ IT66121_SW_RST_REF);
+ if (ret)
+ return ret;
+
+ msleep(50);
+
+ ret = drm_connector_init(bridge->dev, &ctx->connector,
+ &it66121_connector_funcs,
+ DRM_MODE_CONNECTOR_HDMIA);
+ if (ret)
+ return ret;
+
+ ctx->connector.polled = DRM_CONNECTOR_POLL_HPD;
+ drm_connector_helper_add(&ctx->connector,
+ &it66121_connector_helper_funcs);
+
+ ret = drm_connector_attach_encoder(&ctx->connector, bridge->encoder);
+ if (ret)
+ return ret;
+
+ ret = drm_connector_register(&ctx->connector);
+ if (ret)
+ return ret;
+
+ /* Start interrupts */
+ return regmap_write_bits(ctx->regmap, IT66121_INT_MASK1_REG,
+ IT66121_INT_MASK1_DDC_NOACK |
+ IT66121_INT_MASK1_HPD |
+ IT66121_INT_MASK1_DDC_FIFOERR |
+ IT66121_INT_MASK1_DDC_BUSHANG,
+ ~(IT66121_INT_MASK1_DDC_NOACK |
+ IT66121_INT_MASK1_HPD |
+ IT66121_INT_MASK1_DDC_FIFOERR |
+ IT66121_INT_MASK1_DDC_BUSHANG) & 0xFF);
+}
+
+static int it66121_set_mute(struct it66121_ctx *ctx, bool mute)
+{
+ int ret;
+ unsigned int val;
+
+ val = mute ? IT66121_AV_MUTE_ON : (~IT66121_AV_MUTE_ON & 0xFF);
+ ret = regmap_write_bits(ctx->regmap, IT66121_AV_MUTE_REG,
+ IT66121_AV_MUTE_ON, val);
+ if (ret)
+ return ret;
+
+ return regmap_write(ctx->regmap, IT66121_PKT_GEN_CTRL_REG,
+ IT66121_PKT_GEN_CTRL_ON |
+ IT66121_PKT_GEN_CTRL_RPT);
+}
+
+static void it66121_bridge_enable(struct drm_bridge *bridge)
+{
+ struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx,
+ bridge);
+
+ it66121_set_mute(ctx, false);
+}
+
+static void it66121_bridge_disable(struct drm_bridge *bridge)
+{
+ struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx,
+ bridge);
+
+ it66121_set_mute(ctx, true);
+}
+
+static
+void it66121_bridge_mode_set(struct drm_bridge *bridge,
+ const struct drm_display_mode *mode,
+ const struct drm_display_mode *adjusted_mode)
+{
+ int ret, i;
+ u8 buf[HDMI_INFOFRAME_SIZE(AVI)];
+ struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx,
+ bridge);
+ const u16 aviinfo_reg[HDMI_AVI_INFOFRAME_SIZE] = {
+ IT66121_AVIINFO_DB1_REG,
+ IT66121_AVIINFO_DB2_REG,
+ IT66121_AVIINFO_DB3_REG,
+ IT66121_AVIINFO_DB4_REG,
+ IT66121_AVIINFO_DB5_REG,
+ IT66121_AVIINFO_DB6_REG,
+ IT66121_AVIINFO_DB7_REG,
+ IT66121_AVIINFO_DB8_REG,
+ IT66121_AVIINFO_DB9_REG,
+ IT66121_AVIINFO_DB10_REG,
+ IT66121_AVIINFO_DB11_REG,
+ IT66121_AVIINFO_DB12_REG,
+ IT66121_AVIINFO_DB13_REG
+ };
+
+ mutex_lock(&ctx->lock);
+
+ hdmi_avi_infoframe_init(&ctx->hdmi_avi_infoframe);
+
+ ret = drm_hdmi_avi_infoframe_from_display_mode(&ctx->hdmi_avi_infoframe,
+ &ctx->connector,
+ adjusted_mode);
+ if (ret) {
+ DRM_ERROR("Failed to setup AVI infoframe: %d\n", ret);
+ goto unlock;
+ }
+
+ ret = hdmi_avi_infoframe_pack(&ctx->hdmi_avi_infoframe, buf,
+ sizeof(buf));
+ if (ret < 0) {
+ DRM_ERROR("Failed to pack infoframe: %d\n", ret);
+ goto unlock;
+ }
+
+ /* Write new AVI infoframe packet */
+ for (i = 0; i < HDMI_AVI_INFOFRAME_SIZE; i++) {
+ if (regmap_write(ctx->regmap, aviinfo_reg[i],
+ buf[i + HDMI_INFOFRAME_HEADER_SIZE]))
+ goto unlock;
+ }
+ if (regmap_write(ctx->regmap, IT66121_AVIINFO_CSUM_REG, buf[3]))
+ goto unlock;
+
+ /* Enable AVI infoframe */
+ if (regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG,
+ IT66121_AVI_INFO_PKT_ON |
+ IT66121_AVI_INFO_PKT_RPT))
+ goto unlock;
+
+ /* Set TX mode to HDMI */
+ if (regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG,
+ IT66121_HDMI_MODE_HDMI))
+ goto unlock;
+
+ if (regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
+ IT66121_CLK_BANK_PWROFF_TXCLK,
+ IT66121_CLK_BANK_PWROFF_TXCLK))
+ goto unlock;
+
+ if (it66121_configure_input(ctx))
+ goto unlock;
+
+ if (it66121_configure_afe(ctx, adjusted_mode))
+ goto unlock;
+
+ regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
+ IT66121_CLK_BANK_PWROFF_TXCLK,
+ ~IT66121_CLK_BANK_PWROFF_TXCLK & 0xFF);
+
+unlock:
+ mutex_unlock(&ctx->lock);
+}
+
+static const struct drm_bridge_funcs it66121_bridge_funcs = {
+ .attach = it66121_bridge_attach,
+ .enable = it66121_bridge_enable,
+ .disable = it66121_bridge_disable,
+ .mode_set = it66121_bridge_mode_set,
+};
+
+static irqreturn_t it66121_irq_threaded_handler(int irq, void *dev_id)
+{
+ int ret;
+ unsigned int val;
+ struct it66121_ctx *ctx = dev_id;
+ struct device *dev = ctx->dev;
+ bool event = false;
+
+ mutex_lock(&ctx->lock);
+
+ ret = regmap_read(ctx->regmap, IT66121_SYS_STATUS_REG, &val);
+ if (ret)
+ goto unlock;
+
+ if (val & IT66121_SYS_STATUS_ACTIVE_IRQ) {
+ ret = regmap_read(ctx->regmap, IT66121_INT_STATUS1_REG, &val);
+ if (ret) {
+ dev_err(dev, "Cannot read STATUS1_REG %d\n", ret);
+ } else {
+ if (val & IT66121_INT_STATUS1_DDC_FIFOERR)
+ it66121_clear_ddc_fifo(ctx);
+ if (val & (IT66121_INT_STATUS1_DDC_BUSHANG |
+ IT66121_INT_STATUS1_DDC_NOACK))
+ it66121_abort_ddc_ops(ctx);
+ if (val & IT66121_INT_STATUS1_HPD_STATUS) {
+ regmap_write_bits(ctx->regmap,
+ IT66121_INT_CLR1_REG,
+ IT66121_INT_CLR1_HPD,
+ IT66121_INT_CLR1_HPD);
+
+ if (!it66121_is_hpd_detect(ctx)) {
+ kfree(ctx->edid);
+ ctx->edid = NULL;
+ }
+ event = true;
+ }
+ }
+
+ regmap_write_bits(ctx->regmap, IT66121_SYS_STATUS_REG,
+ IT66121_SYS_STATUS_CLEAR_IRQ,
+ IT66121_SYS_STATUS_CLEAR_IRQ);
+ }
+
+unlock:
+ mutex_unlock(&ctx->lock);
+
+ if (event)
+ drm_helper_hpd_irq_event(ctx->bridge.dev);
+
+ return IRQ_HANDLED;
+}
+
+static int it66121_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ u8 ids[4];
+ int i, ret;
+ struct it66121_ctx *ctx;
+ struct device *dev = &client->dev;
+
+ if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+ dev_err(dev, "I2C check functionality failed.\n");
+ return -ENXIO;
+ }
+
+ ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+ if (!ctx)
+ return -ENOMEM;
+
+ ctx->dev = dev;
+ ctx->client = client;
+ i2c_set_clientdata(client, ctx);
+ mutex_init(&ctx->lock);
+ ctx->conf = (struct it66121_conf *)of_device_get_match_data(dev);
+ if (!ctx->conf)
+ return -ENODEV;
+
+ ctx->supplies[0].supply = "vcn33";
+ ctx->supplies[1].supply = "vcn18";
+ ctx->supplies[2].supply = "vrf12";
+ ret = devm_regulator_bulk_get(ctx->dev, 3, ctx->supplies);
+ if (ret) {
+ dev_err(ctx->dev, "regulator_bulk failed\n");
+ return ret;
+ }
+
+ ctx->dual_edge = of_property_read_bool(dev->of_node, "pclk-dual-edge");
+
+ ret = ite66121_power_on(ctx);
+ if (ret)
+ return ret;
+
+ it66121_hw_reset(ctx);
+
+ ctx->regmap = devm_regmap_init_i2c(client, &it66121_regmap_config);
+ if (IS_ERR(ctx->regmap)) {
+ ite66121_power_off(ctx);
+ return PTR_ERR(ctx);
+ }
+
+ for (i = 0; i < 4; i++) {
+ regmap_read(ctx->regmap, i, &ret);
+ ids[i] = ret;
+ }
+
+ if (ids[0] != IT66121_VENDOR_ID0 ||
+ ids[1] != IT66121_VENDOR_ID1 ||
+ ids[2] != IT66121_DEVICE_ID0 ||
+ ((ids[3] & IT66121_DEVICE_MASK) != IT66121_DEVICE_ID1)) {
+ ite66121_power_off(ctx);
+ return -ENODEV;
+ }
+
+ ctx->bridge.funcs = &it66121_bridge_funcs;
+ ctx->bridge.of_node = dev->of_node;
+
+ ret = devm_request_threaded_irq(dev, client->irq, NULL,
+ it66121_irq_threaded_handler,
+ IRQF_SHARED | IRQF_TRIGGER_LOW |
+ IRQF_ONESHOT,
+ dev_name(dev),
+ ctx);
+ if (ret < 0) {
+ dev_err(dev, "Failed to request irq %d:%d\n", client->irq, ret);
+ ite66121_power_off(ctx);
+ return ret;
+ }
+
+ drm_bridge_add(&ctx->bridge);
+
+ return 0;
+}
+
+static int it66121_remove(struct i2c_client *client)
+{
+ struct it66121_ctx *ctx = i2c_get_clientdata(client);
+
+ ite66121_power_off(ctx);
+ drm_bridge_remove(&ctx->bridge);
+ kfree(ctx->edid);
+ mutex_destroy(&ctx->lock);
+
+ return 0;
+}
+
+static const struct of_device_id it66121_dt_match[] = {
+ { .compatible = "ite,it66121",
+ .data = &it66121_conf_simple,
+ },
+ { },
+};
+MODULE_DEVICE_TABLE(of, it66121_dt_match);
+
+static const struct i2c_device_id it66121_id[] = {
+ { "it66121", 0 },
+ { },
+};
+MODULE_DEVICE_TABLE(i2c, it66121_id);
+
+static struct i2c_driver it66121_driver = {
+ .driver = {
+ .name = "it66121",
+ .of_match_table = it66121_dt_match,
+ },
+ .probe = it66121_probe,
+ .remove = it66121_remove,
+ .id_table = it66121_id,
+};
+
+module_i2c_driver(it66121_driver);
+
+MODULE_AUTHOR("Phong LE");
+MODULE_DESCRIPTION("IT66121 HDMI transmitter driver");
+MODULE_LICENSE("GPL v2");
--
2.17.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 4/4] MAINTAINERS: add it66121 HDMI bridge driver entry
2020-03-11 12:51 [PATCH v2 0/4] Add it66121 driver Phong LE
` (2 preceding siblings ...)
2020-03-11 12:51 ` [PATCH v2 3/4] drm: bridge: add it66121 driver Phong LE
@ 2020-03-11 12:51 ` Phong LE
2020-03-11 13:22 ` Andy Shevchenko
3 siblings, 1 reply; 17+ messages in thread
From: Phong LE @ 2020-03-11 12:51 UTC (permalink / raw)
To: narmstrong, airlied, daniel, robh+dt, mark.rutland, a.hajda
Cc: Laurent.pinchart, jonas, jernej.skrabec, sam, mripard,
heiko.stuebner, linus.walleij, stephan, icenowy, broonie,
mchehab+samsung, davem, gregkh, Jonathan.Cameron,
andriy.shevchenko, dri-devel, devicetree, linux-kernel
Add Neil Armstrong and myself as maintainers
Signed-off-by: Phong LE <ple@baylibre.com>
---
MAINTAINERS | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 37c2963226d4..3d722d723686 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8977,6 +8977,14 @@ T: git git://linuxtv.org/anttip/media_tree.git
S: Maintained
F: drivers/media/tuners/it913x*
+ITE IT66121 HDMI BRIDGE DRIVER
+M: Phong LE <ple@baylibre.com>
+M: Neil Armstrong <narmstrong@baylibre.com>
+S: Maintained
+F: drivers/gpu/drm/bridge/ite-it66121.c
+T: git git://anongit.freedesktop.org/drm/drm-misc
+F: Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
+
IVTV VIDEO4LINUX DRIVER
M: Andy Walls <awalls@md.metrocast.net>
L: linux-media@vger.kernel.org
--
2.17.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 4/4] MAINTAINERS: add it66121 HDMI bridge driver entry
2020-03-11 12:51 ` [PATCH v2 4/4] MAINTAINERS: add it66121 HDMI bridge driver entry Phong LE
@ 2020-03-11 13:22 ` Andy Shevchenko
0 siblings, 0 replies; 17+ messages in thread
From: Andy Shevchenko @ 2020-03-11 13:22 UTC (permalink / raw)
To: Phong LE
Cc: narmstrong, airlied, daniel, robh+dt, mark.rutland, a.hajda,
Laurent.pinchart, jonas, jernej.skrabec, sam, mripard,
heiko.stuebner, linus.walleij, stephan, icenowy, broonie,
mchehab+samsung, davem, gregkh, Jonathan.Cameron, dri-devel,
devicetree, linux-kernel
On Wed, Mar 11, 2020 at 01:51:35PM +0100, Phong LE wrote:
> Add Neil Armstrong and myself as maintainers
Run parse-maintainers.pl to fix issues.
> Signed-off-by: Phong LE <ple@baylibre.com>
> ---
> MAINTAINERS | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 37c2963226d4..3d722d723686 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8977,6 +8977,14 @@ T: git git://linuxtv.org/anttip/media_tree.git
> S: Maintained
> F: drivers/media/tuners/it913x*
>
> +ITE IT66121 HDMI BRIDGE DRIVER
> +M: Phong LE <ple@baylibre.com>
> +M: Neil Armstrong <narmstrong@baylibre.com>
> +S: Maintained
> +F: drivers/gpu/drm/bridge/ite-it66121.c
> +T: git git://anongit.freedesktop.org/drm/drm-misc
> +F: Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> +
> IVTV VIDEO4LINUX DRIVER
> M: Andy Walls <awalls@md.metrocast.net>
> L: linux-media@vger.kernel.org
> --
> 2.17.1
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/4] drm: bridge: add it66121 driver
2020-03-11 12:51 ` [PATCH v2 3/4] drm: bridge: add it66121 driver Phong LE
@ 2020-03-11 13:55 ` Andy Shevchenko
2020-03-12 0:53 ` Laurent Pinchart
2020-03-13 13:55 ` Laurent Pinchart
2020-06-11 15:22 ` Drew Fustini
2 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2020-03-11 13:55 UTC (permalink / raw)
To: Phong LE
Cc: narmstrong, airlied, daniel, robh+dt, mark.rutland, a.hajda,
Laurent.pinchart, jonas, jernej.skrabec, sam, mripard,
heiko.stuebner, linus.walleij, stephan, icenowy, broonie,
mchehab+samsung, davem, gregkh, Jonathan.Cameron, dri-devel,
devicetree, linux-kernel
On Wed, Mar 11, 2020 at 01:51:34PM +0100, Phong LE wrote:
> This commit is a simple driver for bridge HMDI it66121.
> The input format is RBG and there is no color conversion.
> Audio, HDCP and CEC are not supported yet.
I guess you should have been told in your company how to use get_maintainer.pl
to avoid spamming people.
Hint:
scripts/get_maintainer.pl --git --git-min-percent=67 ...
...
> + * Copyright (C) 2020 BayLibre, SAS
> + * Author: Phong LE <ple@baylibre.com>
> + * Copyright (C) 2018-2019, Artem Mygaiev
> + * Copyright (C) 2017, Fresco Logic, Incorporated.
This too compact to read, perhaps squeeze blank line after your (c) note
followed by description how the rest (c) appear in the file.
> + *
Redundant line.
> + */
...
> +#include <linux/of.h>
This can be replaced with property.h, see comments against ->probe().
> +#include <linux/of_device.h>
> +#include <linux/of_gpio.h>
I didn't find evidence of use of any of those.
...
> +#define IT66121_MASTER_SEL_REG 0x10
> +#define IT66121_AFE_DRV_REG 0x61
> +#define IT66121_INPUT_MODE_REG 0x70
> +#define IT66121_INPUT_CSC_REG 0x72
> +#define IT66121_AFE_XP_REG 0x62
> +#define IT66121_AFE_IP_REG 0x64
> +#define IT66121_AFE_XP_EC1_REG 0x68
> +#define IT66121_SW_RST_REG 0x04
> +#define IT66121_DDC_COMMAND_REG 0x15
> +#define IT66121_HDCP_REG 0x20
> +#define IT66121_INT_STATUS1_REG 0x06
> +#define IT66121_DDC_HEADER_REG 0x11
> +#define IT66121_DDC_OFFSET_REG 0x12
> +#define IT66121_DDC_BYTE_REG 0x13
> +#define IT66121_DDC_SEGMENT_REG 0x14
> +#define IT66121_DDC_RD_FIFO_REG 0x17
> +#define IT66121_CLK_BANK_REG 0x0F
> +#define IT66121_INT_REG 0x05
> +#define IT66121_INT_MASK1_REG 0x09
> +#define IT66121_INT_CLR1_REG 0x0C
> +#define IT66121_AV_MUTE_REG 0xC1
> +#define IT66121_PKT_GEN_CTRL_REG 0xC6
> +#define IT66121_AVIINFO_DB1_REG 0x158
> +#define IT66121_AVIINFO_DB2_REG 0x159
> +#define IT66121_AVIINFO_DB3_REG 0x15A
> +#define IT66121_AVIINFO_DB4_REG 0x15B
> +#define IT66121_AVIINFO_DB5_REG 0x15C
> +#define IT66121_AVIINFO_CSUM_REG 0x15D
> +#define IT66121_AVIINFO_DB6_REG 0x15E
> +#define IT66121_AVIINFO_DB7_REG 0x15F
> +#define IT66121_AVIINFO_DB8_REG 0x160
> +#define IT66121_AVIINFO_DB9_REG 0x161
> +#define IT66121_AVIINFO_DB10_REG 0x162
> +#define IT66121_AVIINFO_DB11_REG 0x163
> +#define IT66121_AVIINFO_DB12_REG 0x164
> +#define IT66121_AVIINFO_DB13_REG 0x165
> +
> +#define IT66121_AVI_INFO_PKT_REG 0xCD
> +#define IT66121_HDMI_MODE_REG 0xC0
> +#define IT66121_SYS_STATUS_REG 0x0E
> +#define IT66121_DDC_STATUS_REG 0x16
It's better to
a) keep register sorted by value (easy to be oriented);
b) have them in fixed width, e.g. 0x0CD.
...
> +#define IT66121_DEVICE_MASK 0x0F
GENMASK() ?
> +#define IT66121_EDID_SLEEP 20000
> +#define IT66121_EDID_TIMEOUT 200000
Care to add units?
> +#define IT66121_AFE_CLK_HIGH 80000
Also, what is the unit of this?
...
> + return regmap_write(ctx->regmap, IT66121_MASTER_SEL_REG,
> + IT66121_MASTER_SEL_HOST);
Indentation?
Same for other similar places.
...
> +static int it66121_configure_afe(struct it66121_ctx *ctx,
> + const struct drm_display_mode *mode)
...like this.
...
> + if (val & (IT66121_DDC_STATUS_NOACK | IT66121_DDC_STATUS_WAIT_BUS |
> + IT66121_DDC_STATUS_ARBI_LOSE))
> + return -EAGAIN;
Perhaps better to
u32 busy = IT66121_DDC_STATUS_NOACK | IT66121_DDC_STATUS_WAIT_BUS |
IT66121_DDC_STATUS_ARBI_LOSE;
if (val & busy)
return -EAGAIN;
?
> +
> + return 0;
> +}
...
> +static int it66121_abort_ddc_ops(struct it66121_ctx *ctx)
> +{
> + int ret;
> + unsigned int swreset, cpdesire;
> +
> + ret = regmap_read(ctx->regmap, IT66121_SW_RST_REG, &swreset);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(ctx->regmap, IT66121_HDCP_REG, &cpdesire);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(ctx->regmap, IT66121_HDCP_REG,
> + cpdesire & (~IT66121_HDCP_CPDESIRED & 0xFF));
> + if (ret)
> + return ret;
regmap_update_bits() ?
> +
> + ret = regmap_write(ctx->regmap, IT66121_SW_RST_REG,
> + swreset | IT66121_SW_RST_HDCP);
> + if (ret)
> + return ret;
This should surround the inner update, correct? Otherwise, regmap_update_bits()
can be used as well.
> +}
...
> +static int it66121_get_edid_block(void *context, u8 *buf,
> + unsigned int block, size_t len)
> +{
> + ret = regmap_write(ctx->regmap, IT66121_DDC_COMMAND_REG,
> + IT66121_DDC_COMMAND_EDID_READ);
> + if (ret)
> + return ret;
> +
> + offset += cnt;
> + remain -= cnt;
> + msleep(20);
Should be explained.
> +
> + ret = it66121_wait_ddc_ready(ctx);
> + if (ret)
> + return ret;
> +
> + do {
> + ret = regmap_read(ctx->regmap,
> + IT66121_DDC_RD_FIFO_REG, &val);
> + if (ret)
> + return ret;
> + *(buf++) = val;
> + cnt--;
> + } while (cnt > 0);
I'm wondering if regmap API has a helper for above like cases.
> + }
> +
> + return 0;
> +}
...
> +static int it66121_connector_get_modes(struct drm_connector *connector)
> +{
> + struct it66121_ctx *ctx = container_of(connector, struct it66121_ctx,
> + connector);
Seems like it can be an inline helper and in all places you can use
struct it66121_ctx *ctx = to_it66121_ctx(connector);
> + ret = drm_connector_update_edid_property(connector,
> + ctx->edid);
It quite fits one line, perhaps configure your editor to see 80 limit sharp?
Applies to all similar cases as well.
> + if (ret) {
> + DRM_ERROR("Failed to update EDID property: %d\n", ret);
> + goto unlock;
> + }
> +}
...
> + return (val & IT66121_SYS_STATUS_HPDETECT);
Too many parentheses.
...
> + max_clock = ctx->dual_edge ? 74250 : 148500;
Magic numbers? Also, It seems one definition is enough.
...
> + msleep(50);
Should be explained.
...
> + val = mute ? IT66121_AV_MUTE_ON : (~IT66121_AV_MUTE_ON & 0xFF);
val = mute ? IT66121_AV_MUTE_ON : 0;
> + ret = regmap_write_bits(ctx->regmap, IT66121_AV_MUTE_REG,
> + IT66121_AV_MUTE_ON, val);
...or even
mask = IT66121_AV_MUTE_ON;
val = mute ? mask : 0;
ret = regmap_write_bits(ctx->regmap, IT66121_AV_MUTE_REG, mask, val);
> + if (ret)
> + return ret;
...
> + regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
> + IT66121_CLK_BANK_PWROFF_TXCLK,
> + ~IT66121_CLK_BANK_PWROFF_TXCLK & 0xFF);
Same idea. What the point of all this operations in the value parameter?
Please, fix over the code.
...
> + if (val & IT66121_SYS_STATUS_ACTIVE_IRQ) {
if (!(val & IT66121_SYS_STATUS_ACTIVE_IRQ))
goto unlock;
> + ret = regmap_read(ctx->regmap, IT66121_INT_STATUS1_REG, &val);
> + if (ret) {
> + dev_err(dev, "Cannot read STATUS1_REG %d\n", ret);
> + } else {
> + if (val & IT66121_INT_STATUS1_DDC_FIFOERR)
> + it66121_clear_ddc_fifo(ctx);
> + if (val & (IT66121_INT_STATUS1_DDC_BUSHANG |
> + IT66121_INT_STATUS1_DDC_NOACK))
Indentation.
> + it66121_abort_ddc_ops(ctx);
> + if (val & IT66121_INT_STATUS1_HPD_STATUS) {
> + regmap_write_bits(ctx->regmap,
> + IT66121_INT_CLR1_REG,
> + IT66121_INT_CLR1_HPD,
> + IT66121_INT_CLR1_HPD);
> +
> + if (!it66121_is_hpd_detect(ctx)) {
> + kfree(ctx->edid);
> + ctx->edid = NULL;
> + }
> + event = true;
> + }
> + }
> +
> + regmap_write_bits(ctx->regmap, IT66121_SYS_STATUS_REG,
> + IT66121_SYS_STATUS_CLEAR_IRQ,
> + IT66121_SYS_STATUS_CLEAR_IRQ);
> + }
> +
> +unlock:
> + mutex_unlock(&ctx->lock);
...
> +static int it66121_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + u8 ids[4];
Magic, also see below.
> + int i, ret;
> + struct it66121_ctx *ctx;
> + struct device *dev = &client->dev;
> + ctx->conf = (struct it66121_conf *)of_device_get_match_data(dev);
device_get_match_data()
In any case why explicit casting?
> + if (!ctx->conf)
> + return -ENODEV;
> +
> + ctx->dual_edge = of_property_read_bool(dev->of_node, "pclk-dual-edge");
device_property_read_bool()
> + for (i = 0; i < 4; i++) {
Magic.
> + regmap_read(ctx->regmap, i, &ret);
> + ids[i] = ret;
> + }
> +
> + if (ids[0] != IT66121_VENDOR_ID0 ||
> + ids[1] != IT66121_VENDOR_ID1 ||
> + ids[2] != IT66121_DEVICE_ID0 ||
> + ((ids[3] & IT66121_DEVICE_MASK) != IT66121_DEVICE_ID1)) {
> + ite66121_power_off(ctx);
> + return -ENODEV;
> + }
> +
> + ctx->bridge.funcs = &it66121_bridge_funcs;
> + ctx->bridge.of_node = dev->of_node;
> +
> + ret = devm_request_threaded_irq(dev, client->irq, NULL,
> + it66121_irq_threaded_handler,
> + IRQF_SHARED | IRQF_TRIGGER_LOW |
> + IRQF_ONESHOT,
Shouldn't flags come from appropriate resource provider (DT / ACPI / etc)?
> + dev_name(dev),
> + ctx);
> + if (ret < 0) {
> + dev_err(dev, "Failed to request irq %d:%d\n", client->irq, ret);
> + ite66121_power_off(ctx);
> + return ret;
> + }
> +
> + drm_bridge_add(&ctx->bridge);
> +
> + return 0;
> +}
...
> +static const struct of_device_id it66121_dt_match[] = {
> + { .compatible = "ite,it66121",
> + .data = &it66121_conf_simple,
> + },
> + { },
Terminator line doesn't need comma.
> +};
...
> +static const struct i2c_device_id it66121_id[] = {
> + { "it66121", 0 },
> + { },
Ditto.
> +};
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/4] drm: bridge: add it66121 driver
2020-03-11 13:55 ` Andy Shevchenko
@ 2020-03-12 0:53 ` Laurent Pinchart
2020-03-12 8:40 ` Andy Shevchenko
0 siblings, 1 reply; 17+ messages in thread
From: Laurent Pinchart @ 2020-03-12 0:53 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Phong LE, narmstrong, airlied, daniel, robh+dt, mark.rutland,
a.hajda, jonas, jernej.skrabec, sam, mripard, heiko.stuebner,
linus.walleij, stephan, icenowy, broonie, mchehab+samsung, davem,
gregkh, Jonathan.Cameron, dri-devel, devicetree, linux-kernel
Hi Andy,
On Wed, Mar 11, 2020 at 03:55:35PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 11, 2020 at 01:51:34PM +0100, Phong LE wrote:
> > This commit is a simple driver for bridge HMDI it66121.
> > The input format is RBG and there is no color conversion.
> > Audio, HDCP and CEC are not supported yet.
>
> I guess you should have been told in your company how to use get_maintainer.pl
> to avoid spamming people.
>
> Hint:
> scripts/get_maintainer.pl --git --git-min-percent=67 ...
I didn't even know about those options... I don't think we can't expect
contributors to know about this if it's not even documented in
Documentation/process/. And even in that case, if this is what every
contribution should use by default, then those options should become the
default for the get_maintainer.pl script.
> ...
>
> > + * Copyright (C) 2020 BayLibre, SAS
> > + * Author: Phong LE <ple@baylibre.com>
> > + * Copyright (C) 2018-2019, Artem Mygaiev
> > + * Copyright (C) 2017, Fresco Logic, Incorporated.
>
> This too compact to read, perhaps squeeze blank line after your (c) note
> followed by description how the rest (c) appear in the file.
>
> > + *
>
> Redundant line.
>
> > + */
>
> ...
>
> > +#include <linux/of.h>
>
> This can be replaced with property.h, see comments against ->probe().
>
> > +#include <linux/of_device.h>
> > +#include <linux/of_gpio.h>
>
> I didn't find evidence of use of any of those.
>
> ...
>
> > +#define IT66121_MASTER_SEL_REG 0x10
>
> > +#define IT66121_AFE_DRV_REG 0x61
>
> > +#define IT66121_INPUT_MODE_REG 0x70
>
> > +#define IT66121_INPUT_CSC_REG 0x72
>
> > +#define IT66121_AFE_XP_REG 0x62
>
> > +#define IT66121_AFE_IP_REG 0x64
>
> > +#define IT66121_AFE_XP_EC1_REG 0x68
>
> > +#define IT66121_SW_RST_REG 0x04
>
> > +#define IT66121_DDC_COMMAND_REG 0x15
>
> > +#define IT66121_HDCP_REG 0x20
>
> > +#define IT66121_INT_STATUS1_REG 0x06
>
> > +#define IT66121_DDC_HEADER_REG 0x11
>
> > +#define IT66121_DDC_OFFSET_REG 0x12
> > +#define IT66121_DDC_BYTE_REG 0x13
> > +#define IT66121_DDC_SEGMENT_REG 0x14
> > +#define IT66121_DDC_RD_FIFO_REG 0x17
>
> > +#define IT66121_CLK_BANK_REG 0x0F
>
> > +#define IT66121_INT_REG 0x05
>
> > +#define IT66121_INT_MASK1_REG 0x09
>
> > +#define IT66121_INT_CLR1_REG 0x0C
>
> > +#define IT66121_AV_MUTE_REG 0xC1
>
> > +#define IT66121_PKT_GEN_CTRL_REG 0xC6
>
> > +#define IT66121_AVIINFO_DB1_REG 0x158
> > +#define IT66121_AVIINFO_DB2_REG 0x159
> > +#define IT66121_AVIINFO_DB3_REG 0x15A
> > +#define IT66121_AVIINFO_DB4_REG 0x15B
> > +#define IT66121_AVIINFO_DB5_REG 0x15C
> > +#define IT66121_AVIINFO_CSUM_REG 0x15D
> > +#define IT66121_AVIINFO_DB6_REG 0x15E
> > +#define IT66121_AVIINFO_DB7_REG 0x15F
> > +#define IT66121_AVIINFO_DB8_REG 0x160
> > +#define IT66121_AVIINFO_DB9_REG 0x161
> > +#define IT66121_AVIINFO_DB10_REG 0x162
> > +#define IT66121_AVIINFO_DB11_REG 0x163
> > +#define IT66121_AVIINFO_DB12_REG 0x164
> > +#define IT66121_AVIINFO_DB13_REG 0x165
> > +
> > +#define IT66121_AVI_INFO_PKT_REG 0xCD
>
> > +#define IT66121_HDMI_MODE_REG 0xC0
>
> > +#define IT66121_SYS_STATUS_REG 0x0E
>
> > +#define IT66121_DDC_STATUS_REG 0x16
>
> It's better to
> a) keep register sorted by value (easy to be oriented);
> b) have them in fixed width, e.g. 0x0CD.
>
> ...
>
> > +#define IT66121_DEVICE_MASK 0x0F
>
> GENMASK() ?
>
> > +#define IT66121_EDID_SLEEP 20000
> > +#define IT66121_EDID_TIMEOUT 200000
>
> Care to add units?
>
> > +#define IT66121_AFE_CLK_HIGH 80000
>
> Also, what is the unit of this?
>
> ...
>
> > + return regmap_write(ctx->regmap, IT66121_MASTER_SEL_REG,
> > + IT66121_MASTER_SEL_HOST);
>
> Indentation?
> Same for other similar places.
>
> ...
>
> > +static int it66121_configure_afe(struct it66121_ctx *ctx,
> > + const struct drm_display_mode *mode)
>
> ...like this.
>
> ...
>
> > + if (val & (IT66121_DDC_STATUS_NOACK | IT66121_DDC_STATUS_WAIT_BUS |
> > + IT66121_DDC_STATUS_ARBI_LOSE))
> > + return -EAGAIN;
>
> Perhaps better to
>
> u32 busy = IT66121_DDC_STATUS_NOACK | IT66121_DDC_STATUS_WAIT_BUS |
> IT66121_DDC_STATUS_ARBI_LOSE;
>
>
> if (val & busy)
> return -EAGAIN;
>
> ?
>
> > +
> > + return 0;
> > +}
>
> ...
>
> > +static int it66121_abort_ddc_ops(struct it66121_ctx *ctx)
> > +{
> > + int ret;
> > + unsigned int swreset, cpdesire;
> > +
> > + ret = regmap_read(ctx->regmap, IT66121_SW_RST_REG, &swreset);
> > + if (ret)
> > + return ret;
> > +
>
> > + ret = regmap_read(ctx->regmap, IT66121_HDCP_REG, &cpdesire);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_write(ctx->regmap, IT66121_HDCP_REG,
> > + cpdesire & (~IT66121_HDCP_CPDESIRED & 0xFF));
> > + if (ret)
> > + return ret;
>
> regmap_update_bits() ?
>
> > +
> > + ret = regmap_write(ctx->regmap, IT66121_SW_RST_REG,
> > + swreset | IT66121_SW_RST_HDCP);
> > + if (ret)
> > + return ret;
>
> This should surround the inner update, correct? Otherwise, regmap_update_bits()
> can be used as well.
>
> > +}
>
> ...
>
> > +static int it66121_get_edid_block(void *context, u8 *buf,
> > + unsigned int block, size_t len)
> > +{
>
> > + ret = regmap_write(ctx->regmap, IT66121_DDC_COMMAND_REG,
> > + IT66121_DDC_COMMAND_EDID_READ);
> > + if (ret)
> > + return ret;
> > +
> > + offset += cnt;
> > + remain -= cnt;
>
> > + msleep(20);
>
> Should be explained.
>
> > +
> > + ret = it66121_wait_ddc_ready(ctx);
> > + if (ret)
> > + return ret;
> > +
>
> > + do {
> > + ret = regmap_read(ctx->regmap,
> > + IT66121_DDC_RD_FIFO_REG, &val);
> > + if (ret)
> > + return ret;
> > + *(buf++) = val;
> > + cnt--;
> > + } while (cnt > 0);
>
> I'm wondering if regmap API has a helper for above like cases.
>
> > + }
> > +
> > + return 0;
> > +}
>
> ...
>
> > +static int it66121_connector_get_modes(struct drm_connector *connector)
> > +{
>
> > + struct it66121_ctx *ctx = container_of(connector, struct it66121_ctx,
> > + connector);
>
> Seems like it can be an inline helper and in all places you can use
>
> struct it66121_ctx *ctx = to_it66121_ctx(connector);
>
>
> > + ret = drm_connector_update_edid_property(connector,
> > + ctx->edid);
>
> It quite fits one line, perhaps configure your editor to see 80 limit sharp?
> Applies to all similar cases as well.
>
> > + if (ret) {
> > + DRM_ERROR("Failed to update EDID property: %d\n", ret);
> > + goto unlock;
> > + }
>
> > +}
>
> ...
>
> > + return (val & IT66121_SYS_STATUS_HPDETECT);
>
> Too many parentheses.
>
> ...
>
> > + max_clock = ctx->dual_edge ? 74250 : 148500;
>
> Magic numbers? Also, It seems one definition is enough.
>
> ...
>
> > + msleep(50);
>
> Should be explained.
>
> ...
>
> > + val = mute ? IT66121_AV_MUTE_ON : (~IT66121_AV_MUTE_ON & 0xFF);
>
> val = mute ? IT66121_AV_MUTE_ON : 0;
>
> > + ret = regmap_write_bits(ctx->regmap, IT66121_AV_MUTE_REG,
> > + IT66121_AV_MUTE_ON, val);
>
> ...or even
>
> mask = IT66121_AV_MUTE_ON;
> val = mute ? mask : 0;
>
> ret = regmap_write_bits(ctx->regmap, IT66121_AV_MUTE_REG, mask, val);
>
> > + if (ret)
> > + return ret;
>
> ...
>
> > + regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
> > + IT66121_CLK_BANK_PWROFF_TXCLK,
> > + ~IT66121_CLK_BANK_PWROFF_TXCLK & 0xFF);
>
> Same idea. What the point of all this operations in the value parameter?
> Please, fix over the code.
>
> ...
>
>
> > + if (val & IT66121_SYS_STATUS_ACTIVE_IRQ) {
>
> if (!(val & IT66121_SYS_STATUS_ACTIVE_IRQ))
> goto unlock;
>
> > + ret = regmap_read(ctx->regmap, IT66121_INT_STATUS1_REG, &val);
> > + if (ret) {
> > + dev_err(dev, "Cannot read STATUS1_REG %d\n", ret);
> > + } else {
> > + if (val & IT66121_INT_STATUS1_DDC_FIFOERR)
> > + it66121_clear_ddc_fifo(ctx);
>
> > + if (val & (IT66121_INT_STATUS1_DDC_BUSHANG |
> > + IT66121_INT_STATUS1_DDC_NOACK))
>
> Indentation.
>
> > + it66121_abort_ddc_ops(ctx);
> > + if (val & IT66121_INT_STATUS1_HPD_STATUS) {
> > + regmap_write_bits(ctx->regmap,
> > + IT66121_INT_CLR1_REG,
> > + IT66121_INT_CLR1_HPD,
> > + IT66121_INT_CLR1_HPD);
> > +
> > + if (!it66121_is_hpd_detect(ctx)) {
> > + kfree(ctx->edid);
> > + ctx->edid = NULL;
> > + }
> > + event = true;
> > + }
> > + }
> > +
> > + regmap_write_bits(ctx->regmap, IT66121_SYS_STATUS_REG,
> > + IT66121_SYS_STATUS_CLEAR_IRQ,
> > + IT66121_SYS_STATUS_CLEAR_IRQ);
> > + }
> > +
> > +unlock:
> > + mutex_unlock(&ctx->lock);
>
> ...
>
> > +static int it66121_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
>
> > + u8 ids[4];
>
> Magic, also see below.
>
> > + int i, ret;
> > + struct it66121_ctx *ctx;
> > + struct device *dev = &client->dev;
>
> > + ctx->conf = (struct it66121_conf *)of_device_get_match_data(dev);
>
> device_get_match_data()
>
> In any case why explicit casting?
>
> > + if (!ctx->conf)
> > + return -ENODEV;
> > +
>
> > + ctx->dual_edge = of_property_read_bool(dev->of_node, "pclk-dual-edge");
>
> device_property_read_bool()
>
> > + for (i = 0; i < 4; i++) {
>
> Magic.
>
> > + regmap_read(ctx->regmap, i, &ret);
> > + ids[i] = ret;
> > + }
>
> > +
> > + if (ids[0] != IT66121_VENDOR_ID0 ||
> > + ids[1] != IT66121_VENDOR_ID1 ||
> > + ids[2] != IT66121_DEVICE_ID0 ||
> > + ((ids[3] & IT66121_DEVICE_MASK) != IT66121_DEVICE_ID1)) {
> > + ite66121_power_off(ctx);
> > + return -ENODEV;
> > + }
> > +
> > + ctx->bridge.funcs = &it66121_bridge_funcs;
> > + ctx->bridge.of_node = dev->of_node;
> > +
> > + ret = devm_request_threaded_irq(dev, client->irq, NULL,
> > + it66121_irq_threaded_handler,
>
> > + IRQF_SHARED | IRQF_TRIGGER_LOW |
> > + IRQF_ONESHOT,
>
> Shouldn't flags come from appropriate resource provider (DT / ACPI / etc)?
>
> > + dev_name(dev),
> > + ctx);
> > + if (ret < 0) {
> > + dev_err(dev, "Failed to request irq %d:%d\n", client->irq, ret);
> > + ite66121_power_off(ctx);
> > + return ret;
> > + }
> > +
> > + drm_bridge_add(&ctx->bridge);
> > +
> > + return 0;
> > +}
>
> ...
>
> > +static const struct of_device_id it66121_dt_match[] = {
> > + { .compatible = "ite,it66121",
> > + .data = &it66121_conf_simple,
> > + },
>
> > + { },
>
> Terminator line doesn't need comma.
>
> > +};
>
> ...
>
> > +static const struct i2c_device_id it66121_id[] = {
> > + { "it66121", 0 },
>
> > + { },
>
> Ditto.
>
> > +};
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/4] drm: bridge: add it66121 driver
2020-03-12 0:53 ` Laurent Pinchart
@ 2020-03-12 8:40 ` Andy Shevchenko
0 siblings, 0 replies; 17+ messages in thread
From: Andy Shevchenko @ 2020-03-12 8:40 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Andy Shevchenko, Phong LE, narmstrong, David Airlie,
Daniel Vetter, Rob Herring, Mark Rutland, Andrzej Hajda, jonas,
jernej.skrabec, Sam Ravnborg, Maxime Ripard, heiko.stuebner,
Linus Walleij, Stephan Gerhold, icenowy, Mark Brown,
Mauro Carvalho Chehab, David S. Miller, Greg Kroah-Hartman,
Jonathan Cameron, dri-devel, devicetree,
Linux Kernel Mailing List
On Thu, Mar 12, 2020 at 2:56 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Wed, Mar 11, 2020 at 03:55:35PM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 11, 2020 at 01:51:34PM +0100, Phong LE wrote:
> > > This commit is a simple driver for bridge HMDI it66121.
> > > The input format is RBG and there is no color conversion.
> > > Audio, HDCP and CEC are not supported yet.
> >
> > I guess you should have been told in your company how to use get_maintainer.pl
> > to avoid spamming people.
> >
> > Hint:
> > scripts/get_maintainer.pl --git --git-min-percent=67 ...
> I didn't even know about those options...
Doesn't one usually look at the help of the tool they are using?
How does one know what it does by default?
> I don't think we can't expect
> contributors to know about this if it's not even documented in
> Documentation/process/.
This is indeed not good and there is room for improvement.
> And even in that case, if this is what every
> contribution should use by default, then those options should become the
> default for the get_maintainer.pl script.
I don't think so. There is a common sense rule in play.
By default get_maintainer.pl shows the roles of the people, so, when
one puts the name at least they may read what the role of the
recipient in question. If I see too many people in the list (let's say
4+), I definitely will start looking for the explanation why.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/4] dt-bindings: display: bridge: add it66121 bindings
2020-03-11 12:51 ` [PATCH v2 2/4] dt-bindings: display: bridge: add it66121 bindings Phong LE
@ 2020-03-13 13:40 ` Laurent Pinchart
2020-03-13 14:12 ` Neil Armstrong
2020-03-23 22:09 ` Rob Herring
1 sibling, 1 reply; 17+ messages in thread
From: Laurent Pinchart @ 2020-03-13 13:40 UTC (permalink / raw)
To: Phong LE
Cc: narmstrong, airlied, daniel, robh+dt, mark.rutland, a.hajda,
jonas, jernej.skrabec, sam, mripard, heiko.stuebner,
linus.walleij, stephan, icenowy, broonie, mchehab+samsung, davem,
gregkh, Jonathan.Cameron, andriy.shevchenko, dri-devel,
devicetree, linux-kernel
Hi Phong,
Thank you for the patch.
On Wed, Mar 11, 2020 at 01:51:33PM +0100, Phong LE wrote:
> Add the ITE bridge HDMI it66121 bindings.
>
> Signed-off-by: Phong LE <ple@baylibre.com>
> ---
> .../bindings/display/bridge/ite,it66121.yaml | 98 +++++++++++++++++++
> 1 file changed, 98 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
>
> diff --git a/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> new file mode 100644
> index 000000000000..1717e880d130
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> @@ -0,0 +1,98 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/ite,it66121.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ITE it66121 HDMI bridge Device Tree Bindings
> +
> +maintainers:
> + - Phong LE <ple@baylibre.com>
> + - Neil Armstrong <narmstrong@baylibre.com>
> +
> +description: |
> + The IT66121 is a high-performance and low-power single channel HDMI
> + transmitter, fully compliant with HDMI 1.3a, HDCP 1.2 and backward compatible
> + to DVI 1.0 specifications.
> +
> +properties:
> + compatible:
> + const: ite,it66121
> +
> + reg:
> + maxItems: 1
> + description: base I2C address of the device
> +
> + reset-gpios:
> + maxItems: 1
> + description: GPIO connected to active low reset
> +
> + vrf12-supply:
> + maxItems: 1
> + description: Regulator for 1.2V analog core power.
> +
> + vcn33-supply:
> + maxItems: 1
> + description: Regulator for 3.3V digital core power.
> +
> + vcn18-supply:
> + maxItems: 1
> + description: Regulator for 1.8V IO core power.
> +
> + interrupts:
> + maxItems: 1
> +
> + pclk-dual-edge:
> + maxItems: 1
> + description: enable pclk dual edge mode.
I'm having a bit of trouble understanding how this operates. Looking at
the driver code the property is only taken into account to calculate the
maximum allowed frequency. How is the IT66121 configured for single vs.
dual pixel clock edge mode ?
> +
> + port:
> + type: object
> +
> + properties:
> + endpoint:
> + type: object
> + description: |
> + Input endpoints of the bridge.
> +
> + required:
> + - endpoint
You should have two ports, one for the bridge input, and one for the
bridge output.
> +
> +required:
> + - compatible
> + - reg
> + - reset-gpios
> + - vrf12-supply
> + - vcn33-supply
> + - vcn18-supply
> + - interrupts
> + - port
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + i2c6 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + it66121hdmitx: it66121hdmitx@4c {
> + compatible = "ite,it66121";
> + pinctrl-names = "default";
> + pinctrl-0 = <&ite_pins_default>;
> + vcn33-supply = <&mt6358_vcn33_wifi_reg>;
> + vcn18-supply = <&mt6358_vcn18_reg>;
> + vrf12-supply = <&mt6358_vrf12_reg>;
> + reset-gpios = <&pio 160 1 /* GPIO_ACTIVE_LOW */>;
> + interrupt-parent = <&pio>;
> + interrupts = <4 8 /* IRQ_TYPE_LEVEL_LOW */>;
> + reg = <0x4c>;
> + pclk-dual-edge;
> +
> + port {
> + it66121_in: endpoint {
> + remote-endpoint = <&display_out>;
> + };
> + };
> + };
> + };
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/4] drm: bridge: add it66121 driver
2020-03-11 12:51 ` [PATCH v2 3/4] drm: bridge: add it66121 driver Phong LE
2020-03-11 13:55 ` Andy Shevchenko
@ 2020-03-13 13:55 ` Laurent Pinchart
2020-06-11 15:22 ` Drew Fustini
2 siblings, 0 replies; 17+ messages in thread
From: Laurent Pinchart @ 2020-03-13 13:55 UTC (permalink / raw)
To: Phong LE
Cc: narmstrong, airlied, daniel, robh+dt, mark.rutland, a.hajda,
jonas, jernej.skrabec, sam, mripard, heiko.stuebner,
linus.walleij, stephan, icenowy, broonie, mchehab+samsung, davem,
gregkh, Jonathan.Cameron, andriy.shevchenko, dri-devel,
devicetree, linux-kernel
Hi Phong,
Thank you for the patch.
On Wed, Mar 11, 2020 at 01:51:34PM +0100, Phong LE wrote:
> This commit is a simple driver for bridge HMDI it66121.
> The input format is RBG and there is no color conversion.
> Audio, HDCP and CEC are not supported yet.
>
> Signed-off-by: Phong LE <ple@baylibre.com>
> ---
> drivers/gpu/drm/bridge/Kconfig | 8 +
> drivers/gpu/drm/bridge/Makefile | 1 +
> drivers/gpu/drm/bridge/ite-it66121.c | 997 +++++++++++++++++++++++++++
> 3 files changed, 1006 insertions(+)
> create mode 100644 drivers/gpu/drm/bridge/ite-it66121.c
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index aaed2347ace9..ae57b83f47c8 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -38,6 +38,14 @@ config DRM_DISPLAY_CONNECTOR
> on ARM-based platforms. Saying Y here when this driver is not needed
> will not cause any issue.
>
> +config DRM_ITE_IT66121
> + tristate "ITE IT66121 HDMI bridge"
> + depends on OF
> + select DRM_KMS_HELPER
> + select REGMAP_I2C
> + help
> + Support for ITE IT66121 HDMI bridge.
> +
> config DRM_LVDS_CODEC
> tristate "Transparent LVDS encoders and decoders support"
> depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 6fb062b5b0f0..9fe80e820f64 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -1,6 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0
> obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
> obj-$(CONFIG_DRM_DISPLAY_CONNECTOR) += display-connector.o
> +obj-$(CONFIG_DRM_ITE_IT66121) += ite-it66121.o
> obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o
> obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
> obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> new file mode 100644
> index 000000000000..7e1a90319a6a
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -0,0 +1,997 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2020 BayLibre, SAS
> + * Author: Phong LE <ple@baylibre.com>
> + * Copyright (C) 2018-2019, Artem Mygaiev
> + * Copyright (C) 2017, Fresco Logic, Incorporated.
> + *
> + */
> +
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_gpio.h>
This header is not needed, you should include <gpio/consumer.h> instead.
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_edid.h>
> +#include <drm/drm_modes.h>
> +#include <drm/drm_print.h>
> +#include <drm/drm_probe_helper.h>
> +
> +#define IT66121_MASTER_SEL_REG 0x10
> +#define IT66121_MASTER_SEL_HOST BIT(0)
> +
> +#define IT66121_AFE_DRV_REG 0x61
> +#define IT66121_AFE_DRV_RST BIT(4)
> +#define IT66121_AFE_DRV_PWD BIT(5)
> +
> +#define IT66121_INPUT_MODE_REG 0x70
> +#define IT66121_INPUT_MODE_RGB (0 << 6)
> +#define IT66121_INPUT_MODE_YUV422 BIT(6)
> +#define IT66121_INPUT_MODE_YUV444 (2 << 6)
> +#define IT66121_INPUT_MODE_CCIR656 BIT(4)
> +#define IT66121_INPUT_MODE_SYNCEMB BIT(3)
> +#define IT66121_INPUT_MODE_DDR BIT(2)
> +
> +#define IT66121_INPUT_CSC_REG 0x72
> +#define IT66121_INPUT_CSC_ENDITHER BIT(7)
> +#define IT66121_INPUT_CSC_ENUDFILTER BIT(6)
> +#define IT66121_INPUT_CSC_DNFREE_GO BIT(5)
> +#define IT66121_INPUT_CSC_RGB_TO_YUV 0x02
> +#define IT66121_INPUT_CSC_YUV_TO_RGB 0x03
> +#define IT66121_INPUT_CSC_NO_CONV 0x00
> +
> +#define IT66121_AFE_XP_REG 0x62
> +#define IT66121_AFE_XP_GAINBIT BIT(7)
> +#define IT66121_AFE_XP_PWDPLL BIT(6)
> +#define IT66121_AFE_XP_ENI BIT(5)
> +#define IT66121_AFE_XP_ENO BIT(4)
> +#define IT66121_AFE_XP_RESETB BIT(3)
> +#define IT66121_AFE_XP_PWDI BIT(2)
> +
> +#define IT66121_AFE_IP_REG 0x64
> +#define IT66121_AFE_IP_GAINBIT BIT(7)
> +#define IT66121_AFE_IP_PWDPLL BIT(6)
> +#define IT66121_AFE_IP_CKSEL_05 (0 << 4)
> +#define IT66121_AFE_IP_CKSEL_1 BIT(4)
> +#define IT66121_AFE_IP_CKSEL_2 (2 << 4)
> +#define IT66121_AFE_IP_CKSEL_2OR4 (3 << 4)
> +#define IT66121_AFE_IP_ER0 BIT(3)
> +#define IT66121_AFE_IP_RESETB BIT(2)
> +#define IT66121_AFE_IP_ENC BIT(1)
> +#define IT66121_AFE_IP_EC1 BIT(0)
> +
> +#define IT66121_AFE_XP_EC1_REG 0x68
> +#define IT66121_AFE_XP_EC1_LOWCLK BIT(4)
> +
> +#define IT66121_SW_RST_REG 0x04
> +#define IT66121_SW_RST_REF BIT(5)
> +#define IT66121_SW_RST_AREF BIT(4)
> +#define IT66121_SW_RST_VID BIT(3)
> +#define IT66121_SW_RST_AUD BIT(2)
> +#define IT66121_SW_RST_HDCP BIT(0)
> +
> +#define IT66121_DDC_COMMAND_REG 0x15
> +#define IT66121_DDC_COMMAND_BURST_READ 0x0
> +#define IT66121_DDC_COMMAND_EDID_READ 0x3
> +#define IT66121_DDC_COMMAND_FIFO_CLR 0x9
> +#define IT66121_DDC_COMMAND_SCL_PULSE 0xA
> +#define IT66121_DDC_COMMAND_ABORT 0xF
> +
> +#define IT66121_HDCP_REG 0x20
> +#define IT66121_HDCP_CPDESIRED BIT(0)
> +#define IT66121_HDCP_EN1P1FEAT BIT(1)
> +
> +#define IT66121_INT_STATUS1_REG 0x06
> +#define IT66121_INT_STATUS1_AUD_OVF BIT(7)
> +#define IT66121_INT_STATUS1_DDC_NOACK BIT(5)
> +#define IT66121_INT_STATUS1_DDC_FIFOERR BIT(4)
> +#define IT66121_INT_STATUS1_DDC_BUSHANG BIT(2)
> +#define IT66121_INT_STATUS1_RX_SENS_STATUS BIT(1)
> +#define IT66121_INT_STATUS1_HPD_STATUS BIT(0)
> +
> +#define IT66121_DDC_HEADER_REG 0x11
> +#define IT66121_DDC_HEADER_HDCP 0x74
> +#define IT66121_DDC_HEADER_EDID 0xA0
> +
> +#define IT66121_DDC_OFFSET_REG 0x12
> +#define IT66121_DDC_BYTE_REG 0x13
> +#define IT66121_DDC_SEGMENT_REG 0x14
> +#define IT66121_DDC_RD_FIFO_REG 0x17
> +
> +#define IT66121_CLK_BANK_REG 0x0F
> +#define IT66121_CLK_BANK_PWROFF_RCLK BIT(6)
> +#define IT66121_CLK_BANK_PWROFF_ACLK BIT(5)
> +#define IT66121_CLK_BANK_PWROFF_TXCLK BIT(4)
> +#define IT66121_CLK_BANK_PWROFF_CRCLK BIT(3)
> +#define IT66121_CLK_BANK_0 0
> +#define IT66121_CLK_BANK_1 1
> +
> +#define IT66121_INT_REG 0x05
> +#define IT66121_INT_ACTIVE_HIGH BIT(7)
> +#define IT66121_INT_OPEN_DRAIN BIT(6)
> +#define IT66121_INT_TX_CLK_OFF BIT(0)
> +
> +#define IT66121_INT_MASK1_REG 0x09
> +#define IT66121_INT_MASK1_AUD_OVF BIT(7)
> +#define IT66121_INT_MASK1_DDC_NOACK BIT(5)
> +#define IT66121_INT_MASK1_DDC_FIFOERR BIT(4)
> +#define IT66121_INT_MASK1_DDC_BUSHANG BIT(2)
> +#define IT66121_INT_MASK1_RX_SENS BIT(1)
> +#define IT66121_INT_MASK1_HPD BIT(0)
> +
> +#define IT66121_INT_CLR1_REG 0x0C
> +#define IT66121_INT_CLR1_PKTACP BIT(7)
> +#define IT66121_INT_CLR1_PKTNULL BIT(6)
> +#define IT66121_INT_CLR1_PKTGEN BIT(5)
> +#define IT66121_INT_CLR1_KSVLISTCHK BIT(4)
> +#define IT66121_INT_CLR1_AUTHDONE BIT(3)
> +#define IT66121_INT_CLR1_AUTHFAIL BIT(2)
> +#define IT66121_INT_CLR1_RX_SENS BIT(1)
> +#define IT66121_INT_CLR1_HPD BIT(0)
> +
> +#define IT66121_AV_MUTE_REG 0xC1
> +#define IT66121_AV_MUTE_ON BIT(0)
> +#define IT66121_AV_MUTE_BLUESCR BIT(1)
> +
> +#define IT66121_PKT_GEN_CTRL_REG 0xC6
> +#define IT66121_PKT_GEN_CTRL_ON BIT(0)
> +#define IT66121_PKT_GEN_CTRL_RPT BIT(1)
> +
> +#define IT66121_AVIINFO_DB1_REG 0x158
> +#define IT66121_AVIINFO_DB2_REG 0x159
> +#define IT66121_AVIINFO_DB3_REG 0x15A
> +#define IT66121_AVIINFO_DB4_REG 0x15B
> +#define IT66121_AVIINFO_DB5_REG 0x15C
> +#define IT66121_AVIINFO_CSUM_REG 0x15D
> +#define IT66121_AVIINFO_DB6_REG 0x15E
> +#define IT66121_AVIINFO_DB7_REG 0x15F
> +#define IT66121_AVIINFO_DB8_REG 0x160
> +#define IT66121_AVIINFO_DB9_REG 0x161
> +#define IT66121_AVIINFO_DB10_REG 0x162
> +#define IT66121_AVIINFO_DB11_REG 0x163
> +#define IT66121_AVIINFO_DB12_REG 0x164
> +#define IT66121_AVIINFO_DB13_REG 0x165
> +
> +#define IT66121_AVI_INFO_PKT_REG 0xCD
> +#define IT66121_AVI_INFO_PKT_ON BIT(0)
> +#define IT66121_AVI_INFO_PKT_RPT BIT(1)
> +
> +#define IT66121_HDMI_MODE_REG 0xC0
> +#define IT66121_HDMI_MODE_HDMI BIT(0)
> +
> +#define IT66121_SYS_STATUS_REG 0x0E
> +#define IT66121_SYS_STATUS_ACTIVE_IRQ BIT(7)
> +#define IT66121_SYS_STATUS_HPDETECT BIT(6)
> +#define IT66121_SYS_STATUS_SENDECTECT BIT(5)
> +#define IT66121_SYS_STATUS_VID_STABLE BIT(4)
> +#define IT66121_SYS_STATUS_AUD_CTS_CLR BIT(1)
> +#define IT66121_SYS_STATUS_CLEAR_IRQ BIT(0)
> +
> +#define IT66121_DDC_STATUS_REG 0x16
> +#define IT66121_DDC_STATUS_TX_DONE BIT(7)
> +#define IT66121_DDC_STATUS_ACTIVE BIT(6)
> +#define IT66121_DDC_STATUS_NOACK BIT(5)
> +#define IT66121_DDC_STATUS_WAIT_BUS BIT(4)
> +#define IT66121_DDC_STATUS_ARBI_LOSE BIT(3)
> +#define IT66121_DDC_STATUS_FIFO_FULL BIT(2)
> +#define IT66121_DDC_STATUS_FIFO_EMPTY BIT(1)
> +#define IT66121_DDC_STATUS_FIFO_VALID BIT(0)
> +
> +#define IT66121_VENDOR_ID0 0x54
> +#define IT66121_VENDOR_ID1 0x49
> +#define IT66121_DEVICE_ID0 0x12
> +#define IT66121_DEVICE_ID1 0x06
> +#define IT66121_DEVICE_MASK 0x0F
> +#define IT66121_EDID_SLEEP 20000
> +#define IT66121_EDID_TIMEOUT 200000
> +#define IT66121_EDID_FIFO_SIZE 32
> +#define IT66121_AFE_CLK_HIGH 80000
> +
> +struct it66121_conf {
> + unsigned int input_mode_reg;
> + unsigned int input_conversion_reg;
> +};
> +
> +struct it66121_ctx {
> + struct regmap *regmap;
> + struct drm_bridge bridge;
> + struct drm_connector connector;
> + struct device *dev;
> + struct gpio_desc *gpio_reset;
> + struct i2c_client *client;
> + struct regulator_bulk_data supplies[3];
> + bool dual_edge;
> + const struct it66121_conf *conf;
> + struct mutex lock; /* Protects fields below and device registers */
> + struct edid *edid;
> + struct hdmi_avi_infoframe hdmi_avi_infoframe;
> +};
> +
> +static const struct regmap_range_cfg it66121_regmap_banks[] = {
> + {
> + .name = "it66121",
> + .range_min = 0x00,
> + .range_max = 0x1FF,
> + .selector_reg = IT66121_CLK_BANK_REG,
> + .selector_mask = 0x1,
> + .selector_shift = 0,
> + .window_start = 0x00,
> + .window_len = 0x130,
> + },
> +};
> +
> +static const struct regmap_config it66121_regmap_config = {
> + .val_bits = 8,
> + .reg_bits = 8,
> + .max_register = 0x1FF,
> + .ranges = it66121_regmap_banks,
> + .num_ranges = ARRAY_SIZE(it66121_regmap_banks),
> +};
> +
> +static const struct it66121_conf it66121_conf_simple = {
> + .input_mode_reg = IT66121_INPUT_MODE_RGB | IT66121_INPUT_MODE_DDR,
> + .input_conversion_reg = IT66121_INPUT_CSC_NO_CONV,
> +};
> +
> +static void it66121_hw_reset(struct it66121_ctx *ctx)
> +{
> + gpiod_set_value(ctx->gpio_reset, 1);
> + msleep(20);
> + gpiod_set_value(ctx->gpio_reset, 0);
> +}
> +
> +static int ite66121_power_on(struct it66121_ctx *ctx)
> +{
> + return regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> +}
> +
> +static int ite66121_power_off(struct it66121_ctx *ctx)
> +{
> + return regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> +}
> +
> +static int it66121_preamble_ddc(struct it66121_ctx *ctx)
> +{
> + return regmap_write(ctx->regmap, IT66121_MASTER_SEL_REG,
> + IT66121_MASTER_SEL_HOST);
> +}
> +
> +static int it66121_fire_afe(struct it66121_ctx *ctx)
> +{
> + return regmap_write(ctx->regmap, IT66121_AFE_DRV_REG, 0);
> +}
> +
> +static int it66121_configure_input(struct it66121_ctx *ctx)
> +{
> + int ret;
> +
> + ret = regmap_write(ctx->regmap, IT66121_INPUT_MODE_REG,
> + ctx->conf->input_mode_reg);
> + if (ret)
> + return ret;
> +
> + return regmap_write(ctx->regmap, IT66121_INPUT_CSC_REG,
> + ctx->conf->input_conversion_reg);
> +}
> +
> +/**
> + * it66121_configure_afe() - Configure the analog front end
> + * @ctx: it66121_ctx object
> + *
> + * RETURNS:
> + * zero if success, a negative error code otherwise.
> + */
> +static int it66121_configure_afe(struct it66121_ctx *ctx,
> + const struct drm_display_mode *mode)
> +{
> + int ret;
> +
> + ret = regmap_write(ctx->regmap, IT66121_AFE_DRV_REG,
> + IT66121_AFE_DRV_RST);
> + if (ret)
> + return ret;
> +
> + if (mode->clock > IT66121_AFE_CLK_HIGH) {
> + ret = regmap_write_bits(ctx->regmap, IT66121_AFE_XP_REG,
> + IT66121_AFE_XP_GAINBIT |
> + IT66121_AFE_XP_ENO,
> + IT66121_AFE_XP_GAINBIT);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write_bits(ctx->regmap, IT66121_AFE_IP_REG,
> + IT66121_AFE_IP_GAINBIT |
> + IT66121_AFE_IP_ER0 |
> + IT66121_AFE_IP_EC1,
> + IT66121_AFE_IP_GAINBIT);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write_bits(ctx->regmap, IT66121_AFE_XP_EC1_REG,
> + IT66121_AFE_XP_EC1_LOWCLK, 0x80);
> + if (ret)
> + return ret;
> + } else {
> + ret = regmap_write_bits(ctx->regmap, IT66121_AFE_XP_REG,
> + IT66121_AFE_XP_GAINBIT |
> + IT66121_AFE_XP_ENO,
> + IT66121_AFE_XP_ENO);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write_bits(ctx->regmap, IT66121_AFE_IP_REG,
> + IT66121_AFE_IP_GAINBIT |
> + IT66121_AFE_IP_ER0 |
> + IT66121_AFE_IP_EC1, IT66121_AFE_IP_ER0 |
> + IT66121_AFE_IP_EC1);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write_bits(ctx->regmap, IT66121_AFE_XP_EC1_REG,
> + IT66121_AFE_XP_EC1_LOWCLK,
> + IT66121_AFE_XP_EC1_LOWCLK);
> + if (ret)
> + return ret;
> + }
> +
> + /* Clear reset flags */
> + ret = regmap_write_bits(ctx->regmap, IT66121_SW_RST_REG,
> + IT66121_SW_RST_REF | IT66121_SW_RST_VID,
> + ~(IT66121_SW_RST_REF | IT66121_SW_RST_VID) &
> + 0xFF);
> + if (ret)
> + return ret;
> +
> + return it66121_fire_afe(ctx);
> +}
> +
> +static inline int it66121_wait_ddc_ready(struct it66121_ctx *ctx)
> +{
> + int ret, val;
> +
> + ret = regmap_read_poll_timeout(ctx->regmap, IT66121_DDC_STATUS_REG,
> + val, true,
> + IT66121_EDID_SLEEP,
> + IT66121_EDID_TIMEOUT);
> + if (ret)
> + return ret;
> +
> + if (val & (IT66121_DDC_STATUS_NOACK | IT66121_DDC_STATUS_WAIT_BUS |
> + IT66121_DDC_STATUS_ARBI_LOSE))
> + return -EAGAIN;
> +
> + return 0;
> +}
> +
> +static int it66121_clear_ddc_fifo(struct it66121_ctx *ctx)
> +{
> + int ret;
> +
> + ret = it66121_preamble_ddc(ctx);
> + if (ret)
> + return ret;
> +
> + return regmap_write(ctx->regmap, IT66121_DDC_COMMAND_REG,
> + IT66121_DDC_COMMAND_FIFO_CLR);
> +}
> +
> +static int it66121_abort_ddc_ops(struct it66121_ctx *ctx)
> +{
> + int ret;
> + unsigned int swreset, cpdesire;
> +
> + ret = regmap_read(ctx->regmap, IT66121_SW_RST_REG, &swreset);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(ctx->regmap, IT66121_HDCP_REG, &cpdesire);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(ctx->regmap, IT66121_HDCP_REG,
> + cpdesire & (~IT66121_HDCP_CPDESIRED & 0xFF));
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(ctx->regmap, IT66121_SW_RST_REG,
> + swreset | IT66121_SW_RST_HDCP);
> + if (ret)
> + return ret;
> +
> + ret = it66121_preamble_ddc(ctx);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(ctx->regmap, IT66121_DDC_COMMAND_REG,
> + IT66121_DDC_COMMAND_ABORT);
> + if (ret)
> + return ret;
> +
> + return it66121_wait_ddc_ready(ctx);
> +}
> +
> +static int it66121_get_edid_block(void *context, u8 *buf,
> + unsigned int block, size_t len)
> +{
> + struct it66121_ctx *ctx = context;
> + unsigned int val;
> + int remain = len;
> + int offset = 0;
> + int ret, cnt;
> +
> + offset = (block % 2) * len;
> + block = block / 2;
> +
> + ret = regmap_read(ctx->regmap, IT66121_INT_STATUS1_REG, &val);
> + if (ret)
> + return ret;
> +
> + if (val & IT66121_INT_STATUS1_DDC_BUSHANG) {
> + ret = it66121_abort_ddc_ops(ctx);
> + if (ret)
> + return ret;
> + }
> +
> + ret = it66121_clear_ddc_fifo(ctx);
> + if (ret)
> + return ret;
> +
> + while (remain > 0) {
> + cnt = (remain > IT66121_EDID_FIFO_SIZE) ?
> + IT66121_EDID_FIFO_SIZE : remain;
> + ret = it66121_preamble_ddc(ctx);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(ctx->regmap, IT66121_DDC_COMMAND_REG,
> + IT66121_DDC_COMMAND_FIFO_CLR);
> + if (ret)
> + return ret;
> +
> + ret = it66121_wait_ddc_ready(ctx);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(ctx->regmap, IT66121_INT_STATUS1_REG, &val);
> + if (ret)
> + return ret;
> +
> + if (val & IT66121_INT_STATUS1_DDC_BUSHANG) {
> + ret = it66121_abort_ddc_ops(ctx);
> + if (ret)
> + return ret;
> + }
> +
> + ret = it66121_preamble_ddc(ctx);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(ctx->regmap, IT66121_DDC_HEADER_REG,
> + IT66121_DDC_HEADER_EDID);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(ctx->regmap, IT66121_DDC_OFFSET_REG, offset);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(ctx->regmap, IT66121_DDC_BYTE_REG, cnt);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(ctx->regmap, IT66121_DDC_SEGMENT_REG, block);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(ctx->regmap, IT66121_DDC_COMMAND_REG,
> + IT66121_DDC_COMMAND_EDID_READ);
> + if (ret)
> + return ret;
> +
> + offset += cnt;
> + remain -= cnt;
> + msleep(20);
> +
> + ret = it66121_wait_ddc_ready(ctx);
> + if (ret)
> + return ret;
> +
> + do {
> + ret = regmap_read(ctx->regmap,
> + IT66121_DDC_RD_FIFO_REG, &val);
> + if (ret)
> + return ret;
> + *(buf++) = val;
> + cnt--;
> + } while (cnt > 0);
> + }
> +
> + return 0;
> +}
> +
> +static int it66121_connector_get_modes(struct drm_connector *connector)
> +{
> + int ret, num_modes = 0;
> + struct it66121_ctx *ctx = container_of(connector, struct it66121_ctx,
> + connector);
> +
> + if (ctx->edid)
> + return drm_add_edid_modes(connector, ctx->edid);
> +
> + mutex_lock(&ctx->lock);
> +
> + ctx->edid = drm_do_get_edid(connector, it66121_get_edid_block, ctx);
> + if (!ctx->edid) {
> + DRM_ERROR("Failed to read EDID\n");
> + goto unlock;
> + }
> +
> + ret = drm_connector_update_edid_property(connector,
> + ctx->edid);
> + if (ret) {
> + DRM_ERROR("Failed to update EDID property: %d\n", ret);
> + goto unlock;
> + }
> +
> + num_modes = drm_add_edid_modes(connector, ctx->edid);
> +
> +unlock:
> + mutex_unlock(&ctx->lock);
> +
> + return num_modes;
> +}
> +
> +static bool it66121_is_hpd_detect(struct it66121_ctx *ctx)
> +{
> + int val;
> +
> + if (regmap_read(ctx->regmap, IT66121_SYS_STATUS_REG, &val))
> + return false;
> +
> + return (val & IT66121_SYS_STATUS_HPDETECT);
> +}
> +
> +static int it66121_connector_detect_ctx(struct drm_connector *connector,
> + struct drm_modeset_acquire_ctx *c,
> + bool force)
> +{
> + struct it66121_ctx *ctx = container_of(connector, struct it66121_ctx,
> + connector);
> +
> + return (it66121_is_hpd_detect(ctx)) ?
> + connector_status_connected : connector_status_disconnected;
> +}
> +
> +static enum drm_mode_status
> +it66121_connector_mode_valid(struct drm_connector *connector,
> + struct drm_display_mode *mode)
> +{
> + unsigned long max_clock;
> + struct it66121_ctx *ctx = container_of(connector, struct it66121_ctx,
> + connector);
> +
> + max_clock = ctx->dual_edge ? 74250 : 148500;
> +
> + if (mode->clock > max_clock)
> + return MODE_CLOCK_HIGH;
> +
> + if (mode->clock < 25000)
> + return MODE_CLOCK_LOW;
> +
> + return MODE_OK;
> +}
> +
> +static struct drm_connector_helper_funcs it66121_connector_helper_funcs = {
> + .get_modes = it66121_connector_get_modes,
> + .detect_ctx = it66121_connector_detect_ctx,
> + .mode_valid = it66121_connector_mode_valid,
> +};
> +
> +static const struct drm_connector_funcs it66121_connector_funcs = {
> + .reset = drm_atomic_helper_connector_reset,
> + .fill_modes = drm_helper_probe_single_connector_modes,
> + .destroy = drm_connector_cleanup,
> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +static int it66121_bridge_attach(struct drm_bridge *bridge,
> + enum drm_bridge_attach_flags flags)
> +{
> + int ret;
> + struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx,
> + bridge);
> +
> + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> + DRM_ERROR("Fix bridge driver to make connector optional!");
> + return -EINVAL;
> + }
Please make the connector optional. It's just a matter of returning
before creating the connector in this function, and exposing connector
operations in it66121_bridge_funcs. Ideally the display controller
driver should use the connector operations and create the connector
itself (there's a nice helper for that :-)), so you could drop all
connector code in this driver.
> +
> + if (!bridge->encoder) {
> + DRM_ERROR("Parent encoder object not found");
> + return -ENODEV;
> + }
> +
> + ret = regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
> + IT66121_CLK_BANK_PWROFF_RCLK, 0);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write_bits(ctx->regmap, IT66121_INT_REG,
> + IT66121_INT_TX_CLK_OFF, 0);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write_bits(ctx->regmap, IT66121_AFE_DRV_REG,
> + IT66121_AFE_DRV_PWD, 0);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write_bits(ctx->regmap, IT66121_AFE_XP_REG,
> + IT66121_AFE_XP_PWDI | IT66121_AFE_XP_PWDPLL, 0);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write_bits(ctx->regmap, IT66121_AFE_IP_REG,
> + IT66121_AFE_IP_PWDPLL, 0);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write_bits(ctx->regmap, IT66121_AFE_DRV_REG,
> + IT66121_AFE_DRV_RST, 0);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write_bits(ctx->regmap, IT66121_AFE_XP_REG,
> + IT66121_AFE_XP_RESETB, IT66121_AFE_XP_RESETB);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write_bits(ctx->regmap, IT66121_AFE_IP_REG,
> + IT66121_AFE_IP_RESETB, IT66121_AFE_IP_RESETB);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write_bits(ctx->regmap, IT66121_SW_RST_REG,
> + IT66121_SW_RST_REF,
> + IT66121_SW_RST_REF);
> + if (ret)
> + return ret;
> +
> + msleep(50);
> +
> + ret = drm_connector_init(bridge->dev, &ctx->connector,
> + &it66121_connector_funcs,
> + DRM_MODE_CONNECTOR_HDMIA);
> + if (ret)
> + return ret;
> +
> + ctx->connector.polled = DRM_CONNECTOR_POLL_HPD;
> + drm_connector_helper_add(&ctx->connector,
> + &it66121_connector_helper_funcs);
> +
> + ret = drm_connector_attach_encoder(&ctx->connector, bridge->encoder);
> + if (ret)
> + return ret;
> +
> + ret = drm_connector_register(&ctx->connector);
> + if (ret)
> + return ret;
> +
> + /* Start interrupts */
> + return regmap_write_bits(ctx->regmap, IT66121_INT_MASK1_REG,
> + IT66121_INT_MASK1_DDC_NOACK |
> + IT66121_INT_MASK1_HPD |
> + IT66121_INT_MASK1_DDC_FIFOERR |
> + IT66121_INT_MASK1_DDC_BUSHANG,
> + ~(IT66121_INT_MASK1_DDC_NOACK |
> + IT66121_INT_MASK1_HPD |
> + IT66121_INT_MASK1_DDC_FIFOERR |
> + IT66121_INT_MASK1_DDC_BUSHANG) & 0xFF);
Is there a reason all those writes have to be performed here instead of
at probe time and/or enable time ?
> +}
> +
> +static int it66121_set_mute(struct it66121_ctx *ctx, bool mute)
> +{
> + int ret;
> + unsigned int val;
> +
> + val = mute ? IT66121_AV_MUTE_ON : (~IT66121_AV_MUTE_ON & 0xFF);
> + ret = regmap_write_bits(ctx->regmap, IT66121_AV_MUTE_REG,
> + IT66121_AV_MUTE_ON, val);
> + if (ret)
> + return ret;
> +
> + return regmap_write(ctx->regmap, IT66121_PKT_GEN_CTRL_REG,
> + IT66121_PKT_GEN_CTRL_ON |
> + IT66121_PKT_GEN_CTRL_RPT);
> +}
> +
> +static void it66121_bridge_enable(struct drm_bridge *bridge)
> +{
> + struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx,
> + bridge);
> +
> + it66121_set_mute(ctx, false);
> +}
> +
> +static void it66121_bridge_disable(struct drm_bridge *bridge)
> +{
> + struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx,
> + bridge);
> +
> + it66121_set_mute(ctx, true);
> +}
> +
> +static
> +void it66121_bridge_mode_set(struct drm_bridge *bridge,
> + const struct drm_display_mode *mode,
> + const struct drm_display_mode *adjusted_mode)
> +{
> + int ret, i;
> + u8 buf[HDMI_INFOFRAME_SIZE(AVI)];
> + struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx,
> + bridge);
> + const u16 aviinfo_reg[HDMI_AVI_INFOFRAME_SIZE] = {
> + IT66121_AVIINFO_DB1_REG,
> + IT66121_AVIINFO_DB2_REG,
> + IT66121_AVIINFO_DB3_REG,
> + IT66121_AVIINFO_DB4_REG,
> + IT66121_AVIINFO_DB5_REG,
> + IT66121_AVIINFO_DB6_REG,
> + IT66121_AVIINFO_DB7_REG,
> + IT66121_AVIINFO_DB8_REG,
> + IT66121_AVIINFO_DB9_REG,
> + IT66121_AVIINFO_DB10_REG,
> + IT66121_AVIINFO_DB11_REG,
> + IT66121_AVIINFO_DB12_REG,
> + IT66121_AVIINFO_DB13_REG
> + };
> +
> + mutex_lock(&ctx->lock);
> +
> + hdmi_avi_infoframe_init(&ctx->hdmi_avi_infoframe);
> +
> + ret = drm_hdmi_avi_infoframe_from_display_mode(&ctx->hdmi_avi_infoframe,
> + &ctx->connector,
> + adjusted_mode);
> + if (ret) {
> + DRM_ERROR("Failed to setup AVI infoframe: %d\n", ret);
> + goto unlock;
> + }
> +
> + ret = hdmi_avi_infoframe_pack(&ctx->hdmi_avi_infoframe, buf,
> + sizeof(buf));
> + if (ret < 0) {
> + DRM_ERROR("Failed to pack infoframe: %d\n", ret);
> + goto unlock;
> + }
> +
> + /* Write new AVI infoframe packet */
> + for (i = 0; i < HDMI_AVI_INFOFRAME_SIZE; i++) {
> + if (regmap_write(ctx->regmap, aviinfo_reg[i],
> + buf[i + HDMI_INFOFRAME_HEADER_SIZE]))
> + goto unlock;
> + }
> + if (regmap_write(ctx->regmap, IT66121_AVIINFO_CSUM_REG, buf[3]))
> + goto unlock;
> +
> + /* Enable AVI infoframe */
> + if (regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG,
> + IT66121_AVI_INFO_PKT_ON |
> + IT66121_AVI_INFO_PKT_RPT))
> + goto unlock;
> +
> + /* Set TX mode to HDMI */
> + if (regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG,
> + IT66121_HDMI_MODE_HDMI))
> + goto unlock;
> +
> + if (regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
> + IT66121_CLK_BANK_PWROFF_TXCLK,
> + IT66121_CLK_BANK_PWROFF_TXCLK))
> + goto unlock;
> +
> + if (it66121_configure_input(ctx))
> + goto unlock;
> +
> + if (it66121_configure_afe(ctx, adjusted_mode))
> + goto unlock;
> +
> + regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
> + IT66121_CLK_BANK_PWROFF_TXCLK,
> + ~IT66121_CLK_BANK_PWROFF_TXCLK & 0xFF);
> +
> +unlock:
> + mutex_unlock(&ctx->lock);
> +}
> +
> +static const struct drm_bridge_funcs it66121_bridge_funcs = {
> + .attach = it66121_bridge_attach,
> + .enable = it66121_bridge_enable,
> + .disable = it66121_bridge_disable,
> + .mode_set = it66121_bridge_mode_set,
> +};
> +
> +static irqreturn_t it66121_irq_threaded_handler(int irq, void *dev_id)
> +{
> + int ret;
> + unsigned int val;
> + struct it66121_ctx *ctx = dev_id;
> + struct device *dev = ctx->dev;
> + bool event = false;
> +
> + mutex_lock(&ctx->lock);
> +
> + ret = regmap_read(ctx->regmap, IT66121_SYS_STATUS_REG, &val);
> + if (ret)
> + goto unlock;
> +
> + if (val & IT66121_SYS_STATUS_ACTIVE_IRQ) {
> + ret = regmap_read(ctx->regmap, IT66121_INT_STATUS1_REG, &val);
> + if (ret) {
> + dev_err(dev, "Cannot read STATUS1_REG %d\n", ret);
> + } else {
> + if (val & IT66121_INT_STATUS1_DDC_FIFOERR)
> + it66121_clear_ddc_fifo(ctx);
> + if (val & (IT66121_INT_STATUS1_DDC_BUSHANG |
> + IT66121_INT_STATUS1_DDC_NOACK))
> + it66121_abort_ddc_ops(ctx);
> + if (val & IT66121_INT_STATUS1_HPD_STATUS) {
> + regmap_write_bits(ctx->regmap,
> + IT66121_INT_CLR1_REG,
> + IT66121_INT_CLR1_HPD,
> + IT66121_INT_CLR1_HPD);
> +
> + if (!it66121_is_hpd_detect(ctx)) {
> + kfree(ctx->edid);
> + ctx->edid = NULL;
> + }
> + event = true;
> + }
> + }
> +
> + regmap_write_bits(ctx->regmap, IT66121_SYS_STATUS_REG,
> + IT66121_SYS_STATUS_CLEAR_IRQ,
> + IT66121_SYS_STATUS_CLEAR_IRQ);
> + }
> +
> +unlock:
> + mutex_unlock(&ctx->lock);
> +
> + if (event)
> + drm_helper_hpd_irq_event(ctx->bridge.dev);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int it66121_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + u8 ids[4];
> + int i, ret;
> + struct it66121_ctx *ctx;
> + struct device *dev = &client->dev;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> + dev_err(dev, "I2C check functionality failed.\n");
> + return -ENXIO;
> + }
> +
> + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> + if (!ctx)
> + return -ENOMEM;
> +
> + ctx->dev = dev;
> + ctx->client = client;
> + i2c_set_clientdata(client, ctx);
> + mutex_init(&ctx->lock);
> + ctx->conf = (struct it66121_conf *)of_device_get_match_data(dev);
> + if (!ctx->conf)
> + return -ENODEV;
> +
> + ctx->supplies[0].supply = "vcn33";
> + ctx->supplies[1].supply = "vcn18";
> + ctx->supplies[2].supply = "vrf12";
> + ret = devm_regulator_bulk_get(ctx->dev, 3, ctx->supplies);
> + if (ret) {
> + dev_err(ctx->dev, "regulator_bulk failed\n");
> + return ret;
> + }
> +
> + ctx->dual_edge = of_property_read_bool(dev->of_node, "pclk-dual-edge");
> +
> + ret = ite66121_power_on(ctx);
> + if (ret)
> + return ret;
> +
> + it66121_hw_reset(ctx);
> +
> + ctx->regmap = devm_regmap_init_i2c(client, &it66121_regmap_config);
> + if (IS_ERR(ctx->regmap)) {
> + ite66121_power_off(ctx);
> + return PTR_ERR(ctx);
I would add an error label at the end to handle the power off, but
that's up to you.
> + }
> +
> + for (i = 0; i < 4; i++) {
> + regmap_read(ctx->regmap, i, &ret);
> + ids[i] = ret;
> + }
> +
> + if (ids[0] != IT66121_VENDOR_ID0 ||
> + ids[1] != IT66121_VENDOR_ID1 ||
> + ids[2] != IT66121_DEVICE_ID0 ||
> + ((ids[3] & IT66121_DEVICE_MASK) != IT66121_DEVICE_ID1)) {
Maybe an error message ?
> + ite66121_power_off(ctx);
> + return -ENODEV;
> + }
> +
> + ctx->bridge.funcs = &it66121_bridge_funcs;
> + ctx->bridge.of_node = dev->of_node;
> +
> + ret = devm_request_threaded_irq(dev, client->irq, NULL,
> + it66121_irq_threaded_handler,
> + IRQF_SHARED | IRQF_TRIGGER_LOW |
> + IRQF_ONESHOT,
> + dev_name(dev),
> + ctx);
> + if (ret < 0) {
> + dev_err(dev, "Failed to request irq %d:%d\n", client->irq, ret);
> + ite66121_power_off(ctx);
> + return ret;
> + }
> +
> + drm_bridge_add(&ctx->bridge);
> +
> + return 0;
> +}
> +
> +static int it66121_remove(struct i2c_client *client)
> +{
> + struct it66121_ctx *ctx = i2c_get_clientdata(client);
> +
> + ite66121_power_off(ctx);
> + drm_bridge_remove(&ctx->bridge);
> + kfree(ctx->edid);
> + mutex_destroy(&ctx->lock);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id it66121_dt_match[] = {
> + { .compatible = "ite,it66121",
> + .data = &it66121_conf_simple,
> + },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, it66121_dt_match);
> +
> +static const struct i2c_device_id it66121_id[] = {
> + { "it66121", 0 },
> + { },
> +};
> +MODULE_DEVICE_TABLE(i2c, it66121_id);
> +
> +static struct i2c_driver it66121_driver = {
> + .driver = {
> + .name = "it66121",
> + .of_match_table = it66121_dt_match,
> + },
> + .probe = it66121_probe,
> + .remove = it66121_remove,
> + .id_table = it66121_id,
> +};
> +
> +module_i2c_driver(it66121_driver);
> +
> +MODULE_AUTHOR("Phong LE");
> +MODULE_DESCRIPTION("IT66121 HDMI transmitter driver");
> +MODULE_LICENSE("GPL v2");
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/4] dt-bindings: display: bridge: add it66121 bindings
2020-03-13 13:40 ` Laurent Pinchart
@ 2020-03-13 14:12 ` Neil Armstrong
2020-03-13 14:17 ` Laurent Pinchart
0 siblings, 1 reply; 17+ messages in thread
From: Neil Armstrong @ 2020-03-13 14:12 UTC (permalink / raw)
To: Laurent Pinchart, Phong LE
Cc: airlied, daniel, robh+dt, mark.rutland, a.hajda, jonas,
jernej.skrabec, sam, mripard, heiko.stuebner, linus.walleij,
stephan, icenowy, broonie, mchehab+samsung, davem, gregkh,
Jonathan.Cameron, andriy.shevchenko, dri-devel, devicetree,
linux-kernel
On 13/03/2020 14:40, Laurent Pinchart wrote:
> Hi Phong,
>
> Thank you for the patch.
>
> On Wed, Mar 11, 2020 at 01:51:33PM +0100, Phong LE wrote:
>> Add the ITE bridge HDMI it66121 bindings.
>>
>> Signed-off-by: Phong LE <ple@baylibre.com>
>> ---
>> .../bindings/display/bridge/ite,it66121.yaml | 98 +++++++++++++++++++
>> 1 file changed, 98 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
>> new file mode 100644
>> index 000000000000..1717e880d130
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
>> @@ -0,0 +1,98 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/bridge/ite,it66121.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: ITE it66121 HDMI bridge Device Tree Bindings
>> +
>> +maintainers:
>> + - Phong LE <ple@baylibre.com>
>> + - Neil Armstrong <narmstrong@baylibre.com>
>> +
>> +description: |
>> + The IT66121 is a high-performance and low-power single channel HDMI
>> + transmitter, fully compliant with HDMI 1.3a, HDCP 1.2 and backward compatible
>> + to DVI 1.0 specifications.
>> +
>> +properties:
>> + compatible:
>> + const: ite,it66121
>> +
>> + reg:
>> + maxItems: 1
>> + description: base I2C address of the device
>> +
>> + reset-gpios:
>> + maxItems: 1
>> + description: GPIO connected to active low reset
>> +
>> + vrf12-supply:
>> + maxItems: 1
>> + description: Regulator for 1.2V analog core power.
>> +
>> + vcn33-supply:
>> + maxItems: 1
>> + description: Regulator for 3.3V digital core power.
>> +
>> + vcn18-supply:
>> + maxItems: 1
>> + description: Regulator for 1.8V IO core power.
>> +
>> + interrupts:
>> + maxItems: 1
>> +
>> + pclk-dual-edge:
>> + maxItems: 1
>> + description: enable pclk dual edge mode.
>
> I'm having a bit of trouble understanding how this operates. Looking at
> the driver code the property is only taken into account to calculate the
> maximum allowed frequency. How is the IT66121 configured for single vs.
> dual pixel clock edge mode ?
Dual edge mode is Dual-Data-Rate mode, the normal mode is MEDIA_BUS_FMT_RGB888_1X24 and dual edge is
MEDIA_BUS_FMT_RGB888_2X12_LE (or MEDIA_BUS_FMT_RGB888_2X12_BE, not sure) on a single clock period.
This should be negociated at runtime, but the bus width should be specified somewhere to select
one of the modes.
Neil
>
>> +
>> + port:
>> + type: object
>> +
>> + properties:
>> + endpoint:
>> + type: object
>> + description: |
>> + Input endpoints of the bridge.
>> +
>> + required:
>> + - endpoint
>
> You should have two ports, one for the bridge input, and one for the
> bridge output.
>
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - reset-gpios
>> + - vrf12-supply
>> + - vcn33-supply
>> + - vcn18-supply
>> + - interrupts
>> + - port
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> + i2c6 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + it66121hdmitx: it66121hdmitx@4c {
>> + compatible = "ite,it66121";
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&ite_pins_default>;
>> + vcn33-supply = <&mt6358_vcn33_wifi_reg>;
>> + vcn18-supply = <&mt6358_vcn18_reg>;
>> + vrf12-supply = <&mt6358_vrf12_reg>;
>> + reset-gpios = <&pio 160 1 /* GPIO_ACTIVE_LOW */>;
>> + interrupt-parent = <&pio>;
>> + interrupts = <4 8 /* IRQ_TYPE_LEVEL_LOW */>;
>> + reg = <0x4c>;
>> + pclk-dual-edge;
>> +
>> + port {
>> + it66121_in: endpoint {
>> + remote-endpoint = <&display_out>;
>> + };
>> + };
>> + };
>> + };
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/4] dt-bindings: display: bridge: add it66121 bindings
2020-03-13 14:12 ` Neil Armstrong
@ 2020-03-13 14:17 ` Laurent Pinchart
2020-03-13 15:29 ` Neil Armstrong
0 siblings, 1 reply; 17+ messages in thread
From: Laurent Pinchart @ 2020-03-13 14:17 UTC (permalink / raw)
To: Neil Armstrong
Cc: Phong LE, airlied, daniel, robh+dt, mark.rutland, a.hajda, jonas,
jernej.skrabec, sam, mripard, heiko.stuebner, linus.walleij,
stephan, icenowy, broonie, mchehab+samsung, davem, gregkh,
Jonathan.Cameron, andriy.shevchenko, dri-devel, devicetree,
linux-kernel
Hi Neil,
On Fri, Mar 13, 2020 at 03:12:13PM +0100, Neil Armstrong wrote:
> On 13/03/2020 14:40, Laurent Pinchart wrote:
> > On Wed, Mar 11, 2020 at 01:51:33PM +0100, Phong LE wrote:
> >> Add the ITE bridge HDMI it66121 bindings.
> >>
> >> Signed-off-by: Phong LE <ple@baylibre.com>
> >> ---
> >> .../bindings/display/bridge/ite,it66121.yaml | 98 +++++++++++++++++++
> >> 1 file changed, 98 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> >> new file mode 100644
> >> index 000000000000..1717e880d130
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> >> @@ -0,0 +1,98 @@
> >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/display/bridge/ite,it66121.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: ITE it66121 HDMI bridge Device Tree Bindings
> >> +
> >> +maintainers:
> >> + - Phong LE <ple@baylibre.com>
> >> + - Neil Armstrong <narmstrong@baylibre.com>
> >> +
> >> +description: |
> >> + The IT66121 is a high-performance and low-power single channel HDMI
> >> + transmitter, fully compliant with HDMI 1.3a, HDCP 1.2 and backward compatible
> >> + to DVI 1.0 specifications.
> >> +
> >> +properties:
> >> + compatible:
> >> + const: ite,it66121
> >> +
> >> + reg:
> >> + maxItems: 1
> >> + description: base I2C address of the device
> >> +
> >> + reset-gpios:
> >> + maxItems: 1
> >> + description: GPIO connected to active low reset
> >> +
> >> + vrf12-supply:
> >> + maxItems: 1
> >> + description: Regulator for 1.2V analog core power.
> >> +
> >> + vcn33-supply:
> >> + maxItems: 1
> >> + description: Regulator for 3.3V digital core power.
> >> +
> >> + vcn18-supply:
> >> + maxItems: 1
> >> + description: Regulator for 1.8V IO core power.
> >> +
> >> + interrupts:
> >> + maxItems: 1
> >> +
> >> + pclk-dual-edge:
> >> + maxItems: 1
> >> + description: enable pclk dual edge mode.
> >
> > I'm having a bit of trouble understanding how this operates. Looking at
> > the driver code the property is only taken into account to calculate the
> > maximum allowed frequency. How is the IT66121 configured for single vs.
> > dual pixel clock edge mode ?
>
> Dual edge mode is Dual-Data-Rate mode, the normal mode is MEDIA_BUS_FMT_RGB888_1X24 and dual edge is
> MEDIA_BUS_FMT_RGB888_2X12_LE (or MEDIA_BUS_FMT_RGB888_2X12_BE, not sure) on a single clock period.
>
> This should be negociated at runtime, but the bus width should be specified somewhere to select
> one of the modes.
How about replacing this property by bus-width to report the connected
bus width ? It should then become an endpoint property.
> >> +
> >> + port:
> >> + type: object
> >> +
> >> + properties:
> >> + endpoint:
> >> + type: object
> >> + description: |
> >> + Input endpoints of the bridge.
> >> +
> >> + required:
> >> + - endpoint
> >
> > You should have two ports, one for the bridge input, and one for the
> > bridge output.
> >
> >> +
> >> +required:
> >> + - compatible
> >> + - reg
> >> + - reset-gpios
> >> + - vrf12-supply
> >> + - vcn33-supply
> >> + - vcn18-supply
> >> + - interrupts
> >> + - port
> >> +
> >> +additionalProperties: false
> >> +
> >> +examples:
> >> + - |
> >> + i2c6 {
> >> + #address-cells = <1>;
> >> + #size-cells = <0>;
> >> +
> >> + it66121hdmitx: it66121hdmitx@4c {
> >> + compatible = "ite,it66121";
> >> + pinctrl-names = "default";
> >> + pinctrl-0 = <&ite_pins_default>;
> >> + vcn33-supply = <&mt6358_vcn33_wifi_reg>;
> >> + vcn18-supply = <&mt6358_vcn18_reg>;
> >> + vrf12-supply = <&mt6358_vrf12_reg>;
> >> + reset-gpios = <&pio 160 1 /* GPIO_ACTIVE_LOW */>;
> >> + interrupt-parent = <&pio>;
> >> + interrupts = <4 8 /* IRQ_TYPE_LEVEL_LOW */>;
> >> + reg = <0x4c>;
> >> + pclk-dual-edge;
> >> +
> >> + port {
> >> + it66121_in: endpoint {
> >> + remote-endpoint = <&display_out>;
> >> + };
> >> + };
> >> + };
> >> + };
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/4] dt-bindings: display: bridge: add it66121 bindings
2020-03-13 14:17 ` Laurent Pinchart
@ 2020-03-13 15:29 ` Neil Armstrong
0 siblings, 0 replies; 17+ messages in thread
From: Neil Armstrong @ 2020-03-13 15:29 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Phong LE, airlied, daniel, robh+dt, mark.rutland, a.hajda, jonas,
jernej.skrabec, sam, mripard, heiko.stuebner, linus.walleij,
stephan, icenowy, broonie, mchehab+samsung, davem, gregkh,
Jonathan.Cameron, andriy.shevchenko, dri-devel, devicetree,
linux-kernel
On 13/03/2020 15:17, Laurent Pinchart wrote:
> Hi Neil,
>
> On Fri, Mar 13, 2020 at 03:12:13PM +0100, Neil Armstrong wrote:
>> On 13/03/2020 14:40, Laurent Pinchart wrote:
>>> On Wed, Mar 11, 2020 at 01:51:33PM +0100, Phong LE wrote:
>>>> Add the ITE bridge HDMI it66121 bindings.
>>>>
>>>> Signed-off-by: Phong LE <ple@baylibre.com>
>>>> ---
>>>> .../bindings/display/bridge/ite,it66121.yaml | 98 +++++++++++++++++++
>>>> 1 file changed, 98 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
>>>>
[...]
>>>> +
>>>> + pclk-dual-edge:
>>>> + maxItems: 1
>>>> + description: enable pclk dual edge mode.
>>>
>>> I'm having a bit of trouble understanding how this operates. Looking at
>>> the driver code the property is only taken into account to calculate the
>>> maximum allowed frequency. How is the IT66121 configured for single vs.
>>> dual pixel clock edge mode ?
>>
>> Dual edge mode is Dual-Data-Rate mode, the normal mode is MEDIA_BUS_FMT_RGB888_1X24 and dual edge is
>> MEDIA_BUS_FMT_RGB888_2X12_LE (or MEDIA_BUS_FMT_RGB888_2X12_BE, not sure) on a single clock period.
>>
>> This should be negociated at runtime, but the bus width should be specified somewhere to select
>> one of the modes.
>
> How about replacing this property by bus-width to report the connected
> bus width ? It should then become an endpoint property.
It was my thought.
The mediatek dpi also uses this property, which is also very wrong in the same way.
Neil
[...]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/4] dt-bindings: add ITE vendor
2020-03-11 12:51 ` [PATCH v2 1/4] dt-bindings: add ITE vendor Phong LE
@ 2020-03-23 22:04 ` Rob Herring
0 siblings, 0 replies; 17+ messages in thread
From: Rob Herring @ 2020-03-23 22:04 UTC (permalink / raw)
To: Phong LE
Cc: narmstrong, airlied, daniel, mark.rutland, a.hajda,
Laurent.pinchart, jonas, jernej.skrabec, sam, mripard,
heiko.stuebner, linus.walleij, stephan, icenowy, broonie,
mchehab+samsung, davem, gregkh, Jonathan.Cameron,
andriy.shevchenko, dri-devel, devicetree, linux-kernel
On Wed, Mar 11, 2020 at 01:51:32PM +0100, Phong LE wrote:
> Add ITE Tech Inc. prefix "ite" in vendor-prefixes. More information on:
> http://www.ite.com.tw/
>
> Signed-off-by: Phong LE <ple@baylibre.com>
> ---
> Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
> 1 file changed, 2 insertions(+)
Already have a patch for this queued for 5.7.
Rob
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/4] dt-bindings: display: bridge: add it66121 bindings
2020-03-11 12:51 ` [PATCH v2 2/4] dt-bindings: display: bridge: add it66121 bindings Phong LE
2020-03-13 13:40 ` Laurent Pinchart
@ 2020-03-23 22:09 ` Rob Herring
1 sibling, 0 replies; 17+ messages in thread
From: Rob Herring @ 2020-03-23 22:09 UTC (permalink / raw)
To: Phong LE
Cc: narmstrong, airlied, daniel, mark.rutland, a.hajda,
Laurent.pinchart, jonas, jernej.skrabec, sam, mripard,
heiko.stuebner, linus.walleij, stephan, icenowy, broonie,
mchehab+samsung, davem, gregkh, Jonathan.Cameron,
andriy.shevchenko, dri-devel, devicetree, linux-kernel
On Wed, Mar 11, 2020 at 01:51:33PM +0100, Phong LE wrote:
> Add the ITE bridge HDMI it66121 bindings.
>
> Signed-off-by: Phong LE <ple@baylibre.com>
> ---
> .../bindings/display/bridge/ite,it66121.yaml | 98 +++++++++++++++++++
> 1 file changed, 98 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
>
> diff --git a/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> new file mode 100644
> index 000000000000..1717e880d130
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> @@ -0,0 +1,98 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/ite,it66121.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ITE it66121 HDMI bridge Device Tree Bindings
> +
> +maintainers:
> + - Phong LE <ple@baylibre.com>
> + - Neil Armstrong <narmstrong@baylibre.com>
> +
> +description: |
> + The IT66121 is a high-performance and low-power single channel HDMI
> + transmitter, fully compliant with HDMI 1.3a, HDCP 1.2 and backward compatible
> + to DVI 1.0 specifications.
> +
> +properties:
> + compatible:
> + const: ite,it66121
> +
> + reg:
> + maxItems: 1
> + description: base I2C address of the device
> +
> + reset-gpios:
> + maxItems: 1
> + description: GPIO connected to active low reset
> +
> + vrf12-supply:
> + maxItems: 1
Drop this. Supplies are always a single item.
> + description: Regulator for 1.2V analog core power.
> +
> + vcn33-supply:
> + maxItems: 1
> + description: Regulator for 3.3V digital core power.
> +
> + vcn18-supply:
> + maxItems: 1
> + description: Regulator for 1.8V IO core power.
> +
> + interrupts:
> + maxItems: 1
> +
> + pclk-dual-edge:
> + maxItems: 1
Not a valid property if this is boolean.
> + description: enable pclk dual edge mode.
> +
> + port:
You should have an output port to a connector node.
> + type: object
> +
> + properties:
> + endpoint:
> + type: object
> + description: |
> + Input endpoints of the bridge.
> +
> + required:
> + - endpoint
> +
> +required:
> + - compatible
> + - reg
> + - reset-gpios
> + - vrf12-supply
> + - vcn33-supply
> + - vcn18-supply
> + - interrupts
> + - port
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + i2c6 {
i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + it66121hdmitx: it66121hdmitx@4c {
> + compatible = "ite,it66121";
> + pinctrl-names = "default";
> + pinctrl-0 = <&ite_pins_default>;
> + vcn33-supply = <&mt6358_vcn33_wifi_reg>;
> + vcn18-supply = <&mt6358_vcn18_reg>;
> + vrf12-supply = <&mt6358_vrf12_reg>;
> + reset-gpios = <&pio 160 1 /* GPIO_ACTIVE_LOW */>;
> + interrupt-parent = <&pio>;
> + interrupts = <4 8 /* IRQ_TYPE_LEVEL_LOW */>;
> + reg = <0x4c>;
> + pclk-dual-edge;
> +
> + port {
> + it66121_in: endpoint {
> + remote-endpoint = <&display_out>;
> + };
> + };
> + };
> + };
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/4] drm: bridge: add it66121 driver
2020-03-11 12:51 ` [PATCH v2 3/4] drm: bridge: add it66121 driver Phong LE
2020-03-11 13:55 ` Andy Shevchenko
2020-03-13 13:55 ` Laurent Pinchart
@ 2020-06-11 15:22 ` Drew Fustini
2 siblings, 0 replies; 17+ messages in thread
From: Drew Fustini @ 2020-06-11 15:22 UTC (permalink / raw)
To: Phong LE
Cc: narmstrong, airlied, daniel, robh+dt, mark.rutland, a.hajda,
Laurent.pinchart, jonas, jernej.skrabec, sam, mripard,
heiko.stuebner, linus.walleij, stephan, icenowy, broonie,
mchehab+samsung, davem, gregkh, Jonathan.Cameron,
andriy.shevchenko, dri-devel, devicetree, linux-kernel,
Baozhu Zuo, Jason Kridner, Robert Nelson
On Wed, Mar 11, 2020 at 01:51:34PM +0100, Phong LE wrote:
> This commit is a simple driver for bridge HMDI it66121.
> The input format is RBG and there is no color conversion.
> Audio, HDCP and CEC are not supported yet.
>
> Signed-off-by: Phong LE <ple@baylibre.com>
> ---
> drivers/gpu/drm/bridge/Kconfig | 8 +
> drivers/gpu/drm/bridge/Makefile | 1 +
> drivers/gpu/drm/bridge/ite-it66121.c | 997 +++++++++++++++++++++++++++
> 3 files changed, 1006 insertions(+)
> create mode 100644 drivers/gpu/drm/bridge/ite-it66121.c
Thanks for working on this driver. Do you plan to post a v3?
I am interested as the it66121 is used on the HDMI "cape" expansion
board for the BeagleBone Green by Seeed Studio [0] and is currently
supported by an out-of-tree driver [1]
-drew
[0] https://wiki.seeedstudio.com/BeagleBone_Green_HDMI_Cape/
[1] https://github.com/RobertCNelson/ti-linux-kernel-dev/blob/ti-linux-4.4.y/patches/drivers/it66121/0001-gpu-drm-i2c-suppot-it66121-hdmi-chip.patch
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-06-11 15:22 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-11 12:51 [PATCH v2 0/4] Add it66121 driver Phong LE
2020-03-11 12:51 ` [PATCH v2 1/4] dt-bindings: add ITE vendor Phong LE
2020-03-23 22:04 ` Rob Herring
2020-03-11 12:51 ` [PATCH v2 2/4] dt-bindings: display: bridge: add it66121 bindings Phong LE
2020-03-13 13:40 ` Laurent Pinchart
2020-03-13 14:12 ` Neil Armstrong
2020-03-13 14:17 ` Laurent Pinchart
2020-03-13 15:29 ` Neil Armstrong
2020-03-23 22:09 ` Rob Herring
2020-03-11 12:51 ` [PATCH v2 3/4] drm: bridge: add it66121 driver Phong LE
2020-03-11 13:55 ` Andy Shevchenko
2020-03-12 0:53 ` Laurent Pinchart
2020-03-12 8:40 ` Andy Shevchenko
2020-03-13 13:55 ` Laurent Pinchart
2020-06-11 15:22 ` Drew Fustini
2020-03-11 12:51 ` [PATCH v2 4/4] MAINTAINERS: add it66121 HDMI bridge driver entry Phong LE
2020-03-11 13:22 ` Andy Shevchenko
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).