2020-03-11 12:52:25

by Phong LE

[permalink] [raw]
Subject: [PATCH v2 3/4] drm: bridge: add it66121 driver

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 <[email protected]>
---
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 <[email protected]>
+ * 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


2020-03-11 13:56:16

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] drm: bridge: add it66121 driver

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 <[email protected]>
> + * 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


2020-03-12 00:54:43

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] drm: bridge: add it66121 driver

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 <[email protected]>
> > + * 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

2020-03-12 08:42:50

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] drm: bridge: add it66121 driver

On Thu, Mar 12, 2020 at 2:56 AM Laurent Pinchart
<[email protected]> 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

2020-03-13 13:58:17

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] drm: bridge: add it66121 driver

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 <[email protected]>
> ---
> 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 <[email protected]>
> + * 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

2020-06-11 18:55:11

by Drew Fustini

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] drm: bridge: add it66121 driver

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 <[email protected]>
> ---
> 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