[PATCH 00/10] drm: bridge: it66121: IT6610 support and cleanups

[PATCH 00/10] drm: bridge: it66121: IT6610 support and cleanups

[Paul Cercueil]

Hi,

This patchset adds a few cleanups to the IT66121 HDMI chip driver, and
most importantly adds support for the IT6610 chip.

The driver was tested with both chips, but as I only own a HDMI monitor
without speakers, HDMI audio may not be working on the IT6610.

Cheers,
-Paul

Paul Cercueil (10):
  dt-bindings: display: bridge: it66121: Add compatible string for
    IT6610
  drm: bridge: it66121: Use devm_regulator_bulk_get_enable()
  drm: bridge: it66121: Use regmap_noinc_read()
  drm: bridge: it66121: Write AVI infoframe with regmap_bulk_write()
  drm: bridge: it66121: Fix wait for DDC ready
  drm: bridge: it66121: Don't use DDC error IRQs
  drm: bridge: it66121: Don't clear DDC FIFO twice
  drm: bridge: it66121: Set DDC preamble only once before reading EDID
  drm: bridge: it66121: Move VID/PID to new it66121_chip_info structure
  drm: bridge: it66121: Add support for the IT6610

 .../bindings/display/bridge/ite,it66121.yaml  |   4 +-
 drivers/gpu/drm/bridge/ite-it66121.c          | 315 +++++++++---------
 2 files changed, 157 insertions(+), 162 deletions(-)

-- 
2.35.1

[PATCH 01/10] dt-bindings: display: bridge: it66121: Add compatible string for IT6610

[Paul Cercueil]

Add a new ite,it6610 compatible string to the IT66121 binding
documentation, since the two chips are very similar.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 .../devicetree/bindings/display/bridge/ite,it66121.yaml       | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
index 1b2185be92cd..72957be0ba3c 100644
--- a/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
@@ -17,7 +17,9 @@ description: |
 
 properties:
   compatible:
-    const: ite,it66121
+    enum:
+      - ite,it66121
+      - ite,it6610
 
   reg:
     maxItems: 1
-- 
2.35.1

[PATCH 02/10] drm: bridge: it66121: Use devm_regulator_bulk_get_enable()

[Paul Cercueil]

Simplify the code of the driver by using
devm_regulator_bulk_get_enable(), which will handle powering up the
regulators, and disabling them on probe error or module removal.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/bridge/ite-it66121.c | 34 +++++++---------------------
 1 file changed, 8 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 7476cfbf9585..a698eec8f250 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -301,7 +301,6 @@ struct it66121_ctx {
 	struct device *dev;
 	struct gpio_desc *gpio_reset;
 	struct i2c_client *client;
-	struct regulator_bulk_data supplies[3];
 	u32 bus_width;
 	struct mutex lock; /* Protects fields below and device registers */
 	struct hdmi_avi_infoframe hdmi_avi_infoframe;
@@ -342,16 +341,6 @@ static void it66121_hw_reset(struct it66121_ctx *ctx)
 	gpiod_set_value(ctx->gpio_reset, 0);
 }
 
-static inline int ite66121_power_on(struct it66121_ctx *ctx)
-{
-	return regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
-}
-
-static inline int ite66121_power_off(struct it66121_ctx *ctx)
-{
-	return regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
-}
-
 static inline int it66121_preamble_ddc(struct it66121_ctx *ctx)
 {
 	return regmap_write(ctx->regmap, IT66121_MASTER_SEL_REG, IT66121_MASTER_SEL_HOST);
@@ -1512,6 +1501,10 @@ static int it66121_audio_codec_init(struct it66121_ctx *ctx, struct device *dev)
 	return PTR_ERR_OR_ZERO(ctx->audio.pdev);
 }
 
+static const char * const it66121_supplies[] = {
+	"vcn33", "vcn18", "vrf12"
+};
+
 static int it66121_probe(struct i2c_client *client)
 {
 	u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 };
@@ -1564,26 +1557,18 @@ static int it66121_probe(struct i2c_client *client)
 	i2c_set_clientdata(client, ctx);
 	mutex_init(&ctx->lock);
 
-	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);
+	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(it66121_supplies),
+					     it66121_supplies);
 	if (ret) {
-		dev_err(ctx->dev, "regulator_bulk failed\n");
+		dev_err(dev, "Failed to enable power supplies\n");
 		return ret;
 	}
 
-	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);
+	if (IS_ERR(ctx->regmap))
 		return PTR_ERR(ctx->regmap);
-	}
 
 	regmap_read(ctx->regmap, IT66121_VENDOR_ID0_REG, &vendor_ids[0]);
 	regmap_read(ctx->regmap, IT66121_VENDOR_ID1_REG, &vendor_ids[1]);
@@ -1596,7 +1581,6 @@ static int it66121_probe(struct i2c_client *client)
 
 	if (vendor_ids[0] != IT66121_VENDOR_ID0 || vendor_ids[1] != IT66121_VENDOR_ID1 ||
 	    device_ids[0] != IT66121_DEVICE_ID0 || device_ids[1] != IT66121_DEVICE_ID1) {
-		ite66121_power_off(ctx);
 		return -ENODEV;
 	}
 
@@ -1609,7 +1593,6 @@ static int it66121_probe(struct i2c_client *client)
 					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;
 	}
 
@@ -1626,7 +1609,6 @@ static void it66121_remove(struct i2c_client *client)
 {
 	struct it66121_ctx *ctx = i2c_get_clientdata(client);
 
-	ite66121_power_off(ctx);
 	drm_bridge_remove(&ctx->bridge);
 	mutex_destroy(&ctx->lock);
 }
-- 
2.35.1

[PATCH 03/10] drm: bridge: it66121: Use regmap_noinc_read()

[Paul Cercueil]

Use regmap_noinc_read() instead of reading the data from the DDC FIFO one
byte at a time.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/bridge/ite-it66121.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index a698eec8f250..12222840df30 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -589,13 +589,12 @@ static int it66121_get_edid_block(void *context, u8 *buf,
 		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);
+		ret = regmap_noinc_read(ctx->regmap, IT66121_DDC_RD_FIFO_REG,
+					buf, cnt);
+		if (ret)
+			return ret;
+
+		buf += cnt;
 	}
 
 	return 0;
-- 
2.35.1

[PATCH 04/10] drm: bridge: it66121: Write AVI infoframe with regmap_bulk_write()

[Paul Cercueil]

Since all AVI infoframe registers are contiguous in the address space,
the AVI infoframe can be written in one go with regmap_bulk_write().

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/bridge/ite-it66121.c | 27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 12222840df30..0a4fdfd7af44 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -773,24 +773,9 @@ 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
-	};
+	int ret;
 
 	mutex_lock(&ctx->lock);
 
@@ -810,10 +795,12 @@ void it66121_bridge_mode_set(struct drm_bridge *bridge,
 	}
 
 	/* 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;
-	}
+	ret = regmap_bulk_write(ctx->regmap, IT66121_AVIINFO_DB1_REG,
+				&buf[HDMI_INFOFRAME_HEADER_SIZE],
+				HDMI_AVI_INFOFRAME_SIZE);
+	if (ret)
+		goto unlock;
+
 	if (regmap_write(ctx->regmap, IT66121_AVIINFO_CSUM_REG, buf[3]))
 		goto unlock;
 
-- 
2.35.1

[PATCH 05/10] drm: bridge: it66121: Fix wait for DDC ready

[Paul Cercueil]

The function it66121_wait_ddc_ready() would previously read the status
register until "true", which means it never actually polled anything and
would just read the register once.

Now, it will properly wait until the DDC hardware is ready or until it
reported an error.

The 'busy' variable was also renamed to 'error' since these bits are set
on error and not when the DDC hardware is busy.

Since the DDC ready function is now working properly, the msleep(20) can
be removed.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/bridge/ite-it66121.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 0a4fdfd7af44..bfb9c87a7019 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -440,15 +440,17 @@ static int it66121_configure_afe(struct it66121_ctx *ctx,
 static inline int it66121_wait_ddc_ready(struct it66121_ctx *ctx)
 {
 	int ret, val;
-	u32 busy = IT66121_DDC_STATUS_NOACK | IT66121_DDC_STATUS_WAIT_BUS |
-		   IT66121_DDC_STATUS_ARBI_LOSE;
+	u32 error = IT66121_DDC_STATUS_NOACK | IT66121_DDC_STATUS_WAIT_BUS |
+		    IT66121_DDC_STATUS_ARBI_LOSE;
+	u32 done = IT66121_DDC_STATUS_TX_DONE;
 
-	ret = regmap_read_poll_timeout(ctx->regmap, IT66121_DDC_STATUS_REG, val, true,
-				       IT66121_EDID_SLEEP_US, IT66121_EDID_TIMEOUT_US);
+	ret = regmap_read_poll_timeout(ctx->regmap, IT66121_DDC_STATUS_REG, val,
+				       val & (error | done), IT66121_EDID_SLEEP_US,
+				       IT66121_EDID_TIMEOUT_US);
 	if (ret)
 		return ret;
 
-	if (val & busy)
+	if (val & error)
 		return -EAGAIN;
 
 	return 0;
@@ -582,9 +584,6 @@ static int it66121_get_edid_block(void *context, u8 *buf,
 		offset += cnt;
 		remain -= cnt;
 
-		/* Per programming manual, sleep here before emptying the FIFO */
-		msleep(20);
-
 		ret = it66121_wait_ddc_ready(ctx);
 		if (ret)
 			return ret;
-- 
2.35.1

[PATCH 06/10] drm: bridge: it66121: Don't use DDC error IRQs

[Paul Cercueil]

The DDC error IRQs will fire on the IT6610 every time the FIFO is empty,
which is not very helpful. To resolve this, we can simply disable them,
and handle DDC errors in it66121_wait_ddc_ready().

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/bridge/ite-it66121.c | 49 ++++++----------------------
 1 file changed, 10 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index bfb9c87a7019..06fa59ae5ffc 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -515,16 +515,6 @@ static int it66121_get_edid_block(void *context, u8 *buf,
 	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;
@@ -545,16 +535,6 @@ static int it66121_get_edid_block(void *context, u8 *buf,
 		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;
@@ -585,8 +565,10 @@ static int it66121_get_edid_block(void *context, u8 *buf,
 		remain -= cnt;
 
 		ret = it66121_wait_ddc_ready(ctx);
-		if (ret)
+		if (ret) {
+			it66121_abort_ddc_ops(ctx);
 			return ret;
+		}
 
 		ret = regmap_noinc_read(ctx->regmap, IT66121_DDC_RD_FIFO_REG,
 					buf, cnt);
@@ -671,11 +653,7 @@ static int it66121_bridge_attach(struct drm_bridge *bridge,
 	/* Per programming manual, sleep here for bridge to settle */
 	msleep(50);
 
-	/* Start interrupts */
-	return regmap_write_bits(ctx->regmap, IT66121_INT_MASK1_REG,
-				 IT66121_INT_MASK1_DDC_NOACK |
-				 IT66121_INT_MASK1_DDC_FIFOERR |
-				 IT66121_INT_MASK1_DDC_BUSHANG, 0);
+	return 0;
 }
 
 static int it66121_set_mute(struct it66121_ctx *ctx, bool mute)
@@ -926,21 +904,14 @@ static irqreturn_t it66121_irq_threaded_handler(int irq, void *dev_id)
 	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);
+	} else if (val & IT66121_INT_STATUS1_HPD_STATUS) {
+		regmap_write_bits(ctx->regmap, IT66121_INT_CLR1_REG,
+				  IT66121_INT_CLR1_HPD, IT66121_INT_CLR1_HPD);
 
-			status = it66121_is_hpd_detect(ctx) ? connector_status_connected
-							    : connector_status_disconnected;
+		status = it66121_is_hpd_detect(ctx) ? connector_status_connected
+			: connector_status_disconnected;
 
-			event = true;
-		}
+		event = true;
 	}
 
 	regmap_write_bits(ctx->regmap, IT66121_SYS_STATUS_REG,
-- 
2.35.1

[PATCH 07/10] drm: bridge: it66121: Don't clear DDC FIFO twice

[Paul Cercueil]

The DDC FIFO was cleared before the loop in it66121_get_edid_block(),
and at the beginning of each iteration; which means that it did not have
to be cleared before the loop.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/bridge/ite-it66121.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 06fa59ae5ffc..5335d4abd7c5 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -456,18 +456,6 @@ static inline int it66121_wait_ddc_ready(struct it66121_ctx *ctx)
 	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;
@@ -515,10 +503,6 @@ static int it66121_get_edid_block(void *context, u8 *buf,
 	offset = (block % 2) * len;
 	block = block / 2;
 
-	ret = it66121_clear_ddc_fifo(ctx);
-	if (ret)
-		return ret;
-
 	while (remain > 0) {
 		cnt = (remain > IT66121_EDID_FIFO_SIZE) ?
 				IT66121_EDID_FIFO_SIZE : remain;
-- 
2.35.1

[PATCH 08/10] drm: bridge: it66121: Set DDC preamble only once before reading EDID

[Paul Cercueil]

The DDC preamble and target address only need to be set once before
reading the EDID, even if multiple segments have to be read.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/bridge/ite-it66121.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 5335d4abd7c5..7972003d4776 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -506,9 +506,6 @@ static int it66121_get_edid_block(void *context, u8 *buf,
 	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);
@@ -519,15 +516,6 @@ static int it66121_get_edid_block(void *context, u8 *buf,
 		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;
@@ -842,9 +830,25 @@ static struct edid *it66121_bridge_get_edid(struct drm_bridge *bridge,
 {
 	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
 	struct edid *edid;
+	int ret;
 
 	mutex_lock(&ctx->lock);
+	ret = it66121_preamble_ddc(ctx);
+	if (ret) {
+		edid = ERR_PTR(ret);
+		goto out_unlock;
+	}
+
+	ret = regmap_write(ctx->regmap, IT66121_DDC_HEADER_REG,
+			   IT66121_DDC_HEADER_EDID);
+	if (ret) {
+		edid = ERR_PTR(ret);
+		goto out_unlock;
+	}
+
 	edid = drm_do_get_edid(connector, it66121_get_edid_block, ctx);
+
+out_unlock:
 	mutex_unlock(&ctx->lock);
 
 	return edid;
-- 
2.35.1

[PATCH 09/10] drm: bridge: it66121: Move VID/PID to new it66121_chip_info structure

[Paul Cercueil]

This will make it easier later to introduce support for new chips in
this driver.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/bridge/ite-it66121.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 7972003d4776..43b027b85b8e 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -35,10 +35,6 @@
 #define IT66121_DEVICE_ID0_REG			0x02
 #define IT66121_DEVICE_ID1_REG			0x03
 
-#define IT66121_VENDOR_ID0			0x54
-#define IT66121_VENDOR_ID1			0x49
-#define IT66121_DEVICE_ID0			0x12
-#define IT66121_DEVICE_ID1			0x06
 #define IT66121_REVISION_MASK			GENMASK(7, 4)
 #define IT66121_DEVICE_ID1_MASK			GENMASK(3, 0)
 
@@ -286,13 +282,12 @@
 #define IT66121_AUD_SWL_16BIT			0x2
 #define IT66121_AUD_SWL_NOT_INDICATED		0x0
 
-#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_AFE_CLK_HIGH			80000 /* Khz */
 
+struct it66121_chip_info {
+	u16 vid, pid;
+};
+
 struct it66121_ctx {
 	struct regmap *regmap;
 	struct drm_bridge bridge;
@@ -311,6 +306,7 @@ struct it66121_ctx {
 		u8 swl;
 		bool auto_cts;
 	} audio;
+	const struct it66121_chip_info *info;
 };
 
 static const struct regmap_range_cfg it66121_regmap_banks[] = {
@@ -1451,6 +1447,7 @@ static const char * const it66121_supplies[] = {
 
 static int it66121_probe(struct i2c_client *client)
 {
+	const struct i2c_device_id *id = i2c_client_get_device_id(client);
 	u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 };
 	struct device_node *ep;
 	int ret;
@@ -1472,6 +1469,7 @@ static int it66121_probe(struct i2c_client *client)
 
 	ctx->dev = dev;
 	ctx->client = client;
+	ctx->info = (const struct it66121_chip_info *) id->driver_data;
 
 	of_property_read_u32(ep, "bus-width", &ctx->bus_width);
 	of_node_put(ep);
@@ -1523,8 +1521,8 @@ static int it66121_probe(struct i2c_client *client)
 	revision_id = FIELD_GET(IT66121_REVISION_MASK, device_ids[1]);
 	device_ids[1] &= IT66121_DEVICE_ID1_MASK;
 
-	if (vendor_ids[0] != IT66121_VENDOR_ID0 || vendor_ids[1] != IT66121_VENDOR_ID1 ||
-	    device_ids[0] != IT66121_DEVICE_ID0 || device_ids[1] != IT66121_DEVICE_ID1) {
+	if ((vendor_ids[1] << 8 | vendor_ids[0]) != ctx->info->vid ||
+	    (device_ids[1] << 8 | device_ids[0]) != ctx->info->pid) {
 		return -ENODEV;
 	}
 
@@ -1563,8 +1561,13 @@ static const struct of_device_id it66121_dt_match[] = {
 };
 MODULE_DEVICE_TABLE(of, it66121_dt_match);
 
+static const struct it66121_chip_info it66121_chip_info = {
+	.vid = 0x4954,
+	.pid = 0x0612,
+};
+
 static const struct i2c_device_id it66121_id[] = {
-	{ "it66121", 0 },
+	{ "it66121", (kernel_ulong_t) &it66121_chip_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, it66121_id);
-- 
2.35.1

[PATCH 10/10] drm: bridge: it66121: Add support for the IT6610

[Paul Cercueil]

Add support for the IT6610 HDMI encoder.

The hardware is very similar, and therefore the driver did not require
too many changes. Some bits are only available on the IT66121, and
vice-versa. Also, the IT6610 requires specific polarities on the DE and
pixel lines.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/bridge/ite-it66121.c | 108 +++++++++++++++++++++------
 1 file changed, 86 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 43b027b85b8e..b34860871627 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -68,6 +68,7 @@
 #define IT66121_AFE_XP_ENO			BIT(4)
 #define IT66121_AFE_XP_RESETB			BIT(3)
 #define IT66121_AFE_XP_PWDI			BIT(2)
+#define IT6610_AFE_XP_BYPASS			BIT(0)
 
 #define IT66121_AFE_IP_REG			0x64
 #define IT66121_AFE_IP_GAINBIT			BIT(7)
@@ -284,7 +285,13 @@
 
 #define IT66121_AFE_CLK_HIGH			80000 /* Khz */
 
+enum chip_id {
+	ID_IT6610,
+	ID_IT66121,
+};
+
 struct it66121_chip_info {
+	enum chip_id id;
 	u16 vid, pid;
 };
 
@@ -391,16 +398,22 @@ static int it66121_configure_afe(struct it66121_ctx *ctx,
 
 		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_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;
+		if (ctx->info->id == ID_IT66121) {
+			ret = regmap_write_bits(ctx->regmap, IT66121_AFE_IP_REG,
+						IT66121_AFE_IP_EC1, 0);
+			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 |
@@ -411,17 +424,24 @@ static int it66121_configure_afe(struct it66121_ctx *ctx,
 
 		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);
+					IT66121_AFE_IP_ER0,
+					IT66121_AFE_IP_ER0);
 		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;
+		if (ctx->info->id == ID_IT66121) {
+			ret = regmap_write_bits(ctx->regmap, IT66121_AFE_IP_REG,
+						IT66121_AFE_IP_EC1,
+						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 */
@@ -430,6 +450,14 @@ static int it66121_configure_afe(struct it66121_ctx *ctx,
 	if (ret)
 		return ret;
 
+	if (ctx->info->id == ID_IT6610) {
+		ret = regmap_write_bits(ctx->regmap, IT66121_AFE_XP_REG,
+					IT6610_AFE_XP_BYPASS,
+					IT6610_AFE_XP_BYPASS);
+		if (ret)
+			return ret;
+	}
+
 	return it66121_fire_afe(ctx);
 }
 
@@ -491,7 +519,6 @@ 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;
@@ -572,10 +599,12 @@ static int it66121_bridge_attach(struct drm_bridge *bridge,
 	if (ret)
 		return ret;
 
-	ret = regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
-				IT66121_CLK_BANK_PWROFF_RCLK, 0);
-	if (ret)
-		return ret;
+	if (ctx->info->id == ID_IT66121) {
+		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);
@@ -713,6 +742,24 @@ static void it66121_bridge_disable(struct drm_bridge *bridge,
 	ctx->connector = NULL;
 }
 
+static int it66121_bridge_check(struct drm_bridge *bridge,
+				struct drm_bridge_state *bridge_state,
+				struct drm_crtc_state *crtc_state,
+				struct drm_connector_state *conn_state)
+{
+	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
+
+	if (ctx->info->id == ID_IT6610) {
+		/* The IT6610 only supports these settings */
+		bridge_state->input_bus_cfg.flags |= DRM_BUS_FLAG_DE_HIGH |
+			DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE;
+		bridge_state->input_bus_cfg.flags &=
+			~DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE;
+	}
+
+	return 0;
+}
+
 static
 void it66121_bridge_mode_set(struct drm_bridge *bridge,
 			     const struct drm_display_mode *mode,
@@ -758,9 +805,12 @@ void it66121_bridge_mode_set(struct drm_bridge *bridge,
 	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))
+	if (ctx->info->id == ID_IT66121 &&
+	    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;
@@ -768,7 +818,11 @@ void it66121_bridge_mode_set(struct drm_bridge *bridge,
 	if (it66121_configure_afe(ctx, adjusted_mode))
 		goto unlock;
 
-	regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG, IT66121_CLK_BANK_PWROFF_TXCLK, 0);
+	if (ctx->info->id == ID_IT66121 &&
+	    regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
+			      IT66121_CLK_BANK_PWROFF_TXCLK, 0)) {
+		goto unlock;
+	}
 
 unlock:
 	mutex_unlock(&ctx->lock);
@@ -859,6 +913,7 @@ static const struct drm_bridge_funcs it66121_bridge_funcs = {
 	.atomic_get_input_bus_fmts = it66121_bridge_atomic_get_input_bus_fmts,
 	.atomic_enable = it66121_bridge_enable,
 	.atomic_disable = it66121_bridge_disable,
+	.atomic_check = it66121_bridge_check,
 	.mode_set = it66121_bridge_mode_set,
 	.mode_valid = it66121_bridge_mode_valid,
 	.detect = it66121_bridge_detect,
@@ -1557,17 +1612,26 @@ static void it66121_remove(struct i2c_client *client)
 
 static const struct of_device_id it66121_dt_match[] = {
 	{ .compatible = "ite,it66121" },
+	{ .compatible = "ite,it6610" },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, it66121_dt_match);
 
 static const struct it66121_chip_info it66121_chip_info = {
+	.id = ID_IT66121,
 	.vid = 0x4954,
 	.pid = 0x0612,
 };
 
+static const struct it66121_chip_info it6610_chip_info = {
+	.id = ID_IT6610,
+	.vid = 0xca00,
+	.pid = 0x0611,
+};
+
 static const struct i2c_device_id it66121_id[] = {
 	{ "it66121", (kernel_ulong_t) &it66121_chip_info },
+	{ "it6610", (kernel_ulong_t) &it6610_chip_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, it66121_id);
-- 
2.35.1

Re: [PATCH 01/10] dt-bindings: display: bridge: it66121: Add compatible string for IT6610

[Robert Foss]

On Wed, 14 Dec 2022 at 13:58, Paul Cercueil <paul@crapouillou.net> wrote:
>
> Add a new ite,it6610 compatible string to the IT66121 binding
> documentation, since the two chips are very similar.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  .../devicetree/bindings/display/bridge/ite,it66121.yaml       | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> index 1b2185be92cd..72957be0ba3c 100644
> --- a/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> @@ -17,7 +17,9 @@ description: |
>
>  properties:
>    compatible:
> -    const: ite,it66121
> +    enum:
> +      - ite,it66121
> +      - ite,it6610
>
>    reg:
>      maxItems: 1
> --
> 2.35.1
>

Reviewed-by: Robert Foss <robert.foss@linaro.org>

Re: [PATCH 02/10] drm: bridge: it66121: Use devm_regulator_bulk_get_enable()

[Robert Foss]

On Wed, 14 Dec 2022 at 13:58, Paul Cercueil <paul@crapouillou.net> wrote:
>
> Simplify the code of the driver by using
> devm_regulator_bulk_get_enable(), which will handle powering up the
> regulators, and disabling them on probe error or module removal.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/gpu/drm/bridge/ite-it66121.c | 34 +++++++---------------------
>  1 file changed, 8 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index 7476cfbf9585..a698eec8f250 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -301,7 +301,6 @@ struct it66121_ctx {
>         struct device *dev;
>         struct gpio_desc *gpio_reset;
>         struct i2c_client *client;
> -       struct regulator_bulk_data supplies[3];
>         u32 bus_width;
>         struct mutex lock; /* Protects fields below and device registers */
>         struct hdmi_avi_infoframe hdmi_avi_infoframe;
> @@ -342,16 +341,6 @@ static void it66121_hw_reset(struct it66121_ctx *ctx)
>         gpiod_set_value(ctx->gpio_reset, 0);
>  }
>
> -static inline int ite66121_power_on(struct it66121_ctx *ctx)
> -{
> -       return regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> -}
> -
> -static inline int ite66121_power_off(struct it66121_ctx *ctx)
> -{
> -       return regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> -}
> -
>  static inline int it66121_preamble_ddc(struct it66121_ctx *ctx)
>  {
>         return regmap_write(ctx->regmap, IT66121_MASTER_SEL_REG, IT66121_MASTER_SEL_HOST);
> @@ -1512,6 +1501,10 @@ static int it66121_audio_codec_init(struct it66121_ctx *ctx, struct device *dev)
>         return PTR_ERR_OR_ZERO(ctx->audio.pdev);
>  }
>
> +static const char * const it66121_supplies[] = {
> +       "vcn33", "vcn18", "vrf12"
> +};
> +
>  static int it66121_probe(struct i2c_client *client)
>  {
>         u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 };
> @@ -1564,26 +1557,18 @@ static int it66121_probe(struct i2c_client *client)
>         i2c_set_clientdata(client, ctx);
>         mutex_init(&ctx->lock);
>
> -       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);
> +       ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(it66121_supplies),
> +                                            it66121_supplies);
>         if (ret) {
> -               dev_err(ctx->dev, "regulator_bulk failed\n");
> +               dev_err(dev, "Failed to enable power supplies\n");
>                 return ret;
>         }
>
> -       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);
> +       if (IS_ERR(ctx->regmap))
>                 return PTR_ERR(ctx->regmap);
> -       }
>
>         regmap_read(ctx->regmap, IT66121_VENDOR_ID0_REG, &vendor_ids[0]);
>         regmap_read(ctx->regmap, IT66121_VENDOR_ID1_REG, &vendor_ids[1]);
> @@ -1596,7 +1581,6 @@ static int it66121_probe(struct i2c_client *client)
>
>         if (vendor_ids[0] != IT66121_VENDOR_ID0 || vendor_ids[1] != IT66121_VENDOR_ID1 ||
>             device_ids[0] != IT66121_DEVICE_ID0 || device_ids[1] != IT66121_DEVICE_ID1) {
> -               ite66121_power_off(ctx);
>                 return -ENODEV;
>         }
>
> @@ -1609,7 +1593,6 @@ static int it66121_probe(struct i2c_client *client)
>                                         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;
>         }
>
> @@ -1626,7 +1609,6 @@ static void it66121_remove(struct i2c_client *client)
>  {
>         struct it66121_ctx *ctx = i2c_get_clientdata(client);
>
> -       ite66121_power_off(ctx);
>         drm_bridge_remove(&ctx->bridge);
>         mutex_destroy(&ctx->lock);
>  }
> --
> 2.35.1
>

Reviewed-by: Robert Foss <robert.foss@linaro.org>

Re: [PATCH 03/10] drm: bridge: it66121: Use regmap_noinc_read()

[Robert Foss]

On Wed, 14 Dec 2022 at 13:58, Paul Cercueil <paul@crapouillou.net> wrote:
>
> Use regmap_noinc_read() instead of reading the data from the DDC FIFO one
> byte at a time.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/gpu/drm/bridge/ite-it66121.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index a698eec8f250..12222840df30 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -589,13 +589,12 @@ static int it66121_get_edid_block(void *context, u8 *buf,
>                 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);
> +               ret = regmap_noinc_read(ctx->regmap, IT66121_DDC_RD_FIFO_REG,
> +                                       buf, cnt);
> +               if (ret)
> +                       return ret;
> +
> +               buf += cnt;
>         }
>
>         return 0;
> --
> 2.35.1
>

Reviewed-by: Robert Foss <robert.foss@linaro.org>

Re: [PATCH 04/10] drm: bridge: it66121: Write AVI infoframe with regmap_bulk_write()

[Robert Foss]

On Wed, 14 Dec 2022 at 13:58, Paul Cercueil <paul@crapouillou.net> wrote:
>
> Since all AVI infoframe registers are contiguous in the address space,
> the AVI infoframe can be written in one go with regmap_bulk_write().
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/gpu/drm/bridge/ite-it66121.c | 27 +++++++--------------------
>  1 file changed, 7 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index 12222840df30..0a4fdfd7af44 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -773,24 +773,9 @@ 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
> -       };
> +       int ret;
>
>         mutex_lock(&ctx->lock);
>
> @@ -810,10 +795,12 @@ void it66121_bridge_mode_set(struct drm_bridge *bridge,
>         }
>
>         /* 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;
> -       }
> +       ret = regmap_bulk_write(ctx->regmap, IT66121_AVIINFO_DB1_REG,
> +                               &buf[HDMI_INFOFRAME_HEADER_SIZE],
> +                               HDMI_AVI_INFOFRAME_SIZE);
> +       if (ret)
> +               goto unlock;
> +
>         if (regmap_write(ctx->regmap, IT66121_AVIINFO_CSUM_REG, buf[3]))
>                 goto unlock;
>
> --
> 2.35.1
>

Reviewed-by: Robert Foss <robert.foss@linaro.org>

Re: [PATCH 05/10] drm: bridge: it66121: Fix wait for DDC ready

[Robert Foss]

On Wed, 14 Dec 2022 at 13:59, Paul Cercueil <paul@crapouillou.net> wrote:
>
> The function it66121_wait_ddc_ready() would previously read the status
> register until "true", which means it never actually polled anything and
> would just read the register once.
>
> Now, it will properly wait until the DDC hardware is ready or until it
> reported an error.
>
> The 'busy' variable was also renamed to 'error' since these bits are set
> on error and not when the DDC hardware is busy.
>
> Since the DDC ready function is now working properly, the msleep(20) can
> be removed.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/gpu/drm/bridge/ite-it66121.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index 0a4fdfd7af44..bfb9c87a7019 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -440,15 +440,17 @@ static int it66121_configure_afe(struct it66121_ctx *ctx,
>  static inline int it66121_wait_ddc_ready(struct it66121_ctx *ctx)
>  {
>         int ret, val;
> -       u32 busy = IT66121_DDC_STATUS_NOACK | IT66121_DDC_STATUS_WAIT_BUS |
> -                  IT66121_DDC_STATUS_ARBI_LOSE;
> +       u32 error = IT66121_DDC_STATUS_NOACK | IT66121_DDC_STATUS_WAIT_BUS |
> +                   IT66121_DDC_STATUS_ARBI_LOSE;
> +       u32 done = IT66121_DDC_STATUS_TX_DONE;
>
> -       ret = regmap_read_poll_timeout(ctx->regmap, IT66121_DDC_STATUS_REG, val, true,
> -                                      IT66121_EDID_SLEEP_US, IT66121_EDID_TIMEOUT_US);
> +       ret = regmap_read_poll_timeout(ctx->regmap, IT66121_DDC_STATUS_REG, val,
> +                                      val & (error | done), IT66121_EDID_SLEEP_US,
> +                                      IT66121_EDID_TIMEOUT_US);
>         if (ret)
>                 return ret;
>
> -       if (val & busy)
> +       if (val & error)
>                 return -EAGAIN;
>
>         return 0;
> @@ -582,9 +584,6 @@ static int it66121_get_edid_block(void *context, u8 *buf,
>                 offset += cnt;
>                 remain -= cnt;
>
> -               /* Per programming manual, sleep here before emptying the FIFO */
> -               msleep(20);
> -
>                 ret = it66121_wait_ddc_ready(ctx);
>                 if (ret)
>                         return ret;
> --
> 2.35.1
>

Reviewed-by: Robert Foss <robert.foss@linaro.org>

Re: [PATCH 06/10] drm: bridge: it66121: Don't use DDC error IRQs

[Robert Foss]

On Wed, 14 Dec 2022 at 13:59, Paul Cercueil <paul@crapouillou.net> wrote:
>
> The DDC error IRQs will fire on the IT6610 every time the FIFO is empty,
> which is not very helpful. To resolve this, we can simply disable them,
> and handle DDC errors in it66121_wait_ddc_ready().
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/gpu/drm/bridge/ite-it66121.c | 49 ++++++----------------------
>  1 file changed, 10 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index bfb9c87a7019..06fa59ae5ffc 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -515,16 +515,6 @@ static int it66121_get_edid_block(void *context, u8 *buf,
>         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;
> @@ -545,16 +535,6 @@ static int it66121_get_edid_block(void *context, u8 *buf,
>                 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;
> @@ -585,8 +565,10 @@ static int it66121_get_edid_block(void *context, u8 *buf,
>                 remain -= cnt;
>
>                 ret = it66121_wait_ddc_ready(ctx);
> -               if (ret)
> +               if (ret) {
> +                       it66121_abort_ddc_ops(ctx);
>                         return ret;
> +               }
>
>                 ret = regmap_noinc_read(ctx->regmap, IT66121_DDC_RD_FIFO_REG,
>                                         buf, cnt);
> @@ -671,11 +653,7 @@ static int it66121_bridge_attach(struct drm_bridge *bridge,
>         /* Per programming manual, sleep here for bridge to settle */
>         msleep(50);
>
> -       /* Start interrupts */
> -       return regmap_write_bits(ctx->regmap, IT66121_INT_MASK1_REG,
> -                                IT66121_INT_MASK1_DDC_NOACK |
> -                                IT66121_INT_MASK1_DDC_FIFOERR |
> -                                IT66121_INT_MASK1_DDC_BUSHANG, 0);
> +       return 0;
>  }
>
>  static int it66121_set_mute(struct it66121_ctx *ctx, bool mute)
> @@ -926,21 +904,14 @@ static irqreturn_t it66121_irq_threaded_handler(int irq, void *dev_id)
>         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);
> +       } else if (val & IT66121_INT_STATUS1_HPD_STATUS) {
> +               regmap_write_bits(ctx->regmap, IT66121_INT_CLR1_REG,
> +                                 IT66121_INT_CLR1_HPD, IT66121_INT_CLR1_HPD);
>
> -                       status = it66121_is_hpd_detect(ctx) ? connector_status_connected
> -                                                           : connector_status_disconnected;
> +               status = it66121_is_hpd_detect(ctx) ? connector_status_connected
> +                       : connector_status_disconnected;
>
> -                       event = true;
> -               }
> +               event = true;
>         }
>
>         regmap_write_bits(ctx->regmap, IT66121_SYS_STATUS_REG,
> --
> 2.35.1
>

Reviewed-by: Robert Foss <robert.foss@linaro.org>

Re: [PATCH 07/10] drm: bridge: it66121: Don't clear DDC FIFO twice

[Robert Foss]

On Wed, 14 Dec 2022 at 13:59, Paul Cercueil <paul@crapouillou.net> wrote:
>
> The DDC FIFO was cleared before the loop in it66121_get_edid_block(),
> and at the beginning of each iteration; which means that it did not have
> to be cleared before the loop.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/gpu/drm/bridge/ite-it66121.c | 16 ----------------
>  1 file changed, 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index 06fa59ae5ffc..5335d4abd7c5 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -456,18 +456,6 @@ static inline int it66121_wait_ddc_ready(struct it66121_ctx *ctx)
>         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;
> @@ -515,10 +503,6 @@ static int it66121_get_edid_block(void *context, u8 *buf,
>         offset = (block % 2) * len;
>         block = block / 2;
>
> -       ret = it66121_clear_ddc_fifo(ctx);
> -       if (ret)
> -               return ret;
> -
>         while (remain > 0) {
>                 cnt = (remain > IT66121_EDID_FIFO_SIZE) ?
>                                 IT66121_EDID_FIFO_SIZE : remain;
> --
> 2.35.1
>

Reviewed-by: Robert Foss <robert.foss@linaro.org>

Re: [PATCH 08/10] drm: bridge: it66121: Set DDC preamble only once before reading EDID

[Robert Foss]

On Wed, 14 Dec 2022 at 13:59, Paul Cercueil <paul@crapouillou.net> wrote:
>
> The DDC preamble and target address only need to be set once before
> reading the EDID, even if multiple segments have to be read.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/gpu/drm/bridge/ite-it66121.c | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index 5335d4abd7c5..7972003d4776 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -506,9 +506,6 @@ static int it66121_get_edid_block(void *context, u8 *buf,
>         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);
> @@ -519,15 +516,6 @@ static int it66121_get_edid_block(void *context, u8 *buf,
>                 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;
> @@ -842,9 +830,25 @@ static struct edid *it66121_bridge_get_edid(struct drm_bridge *bridge,
>  {
>         struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
>         struct edid *edid;
> +       int ret;
>
>         mutex_lock(&ctx->lock);
> +       ret = it66121_preamble_ddc(ctx);
> +       if (ret) {
> +               edid = ERR_PTR(ret);
> +               goto out_unlock;
> +       }
> +
> +       ret = regmap_write(ctx->regmap, IT66121_DDC_HEADER_REG,
> +                          IT66121_DDC_HEADER_EDID);
> +       if (ret) {
> +               edid = ERR_PTR(ret);
> +               goto out_unlock;
> +       }
> +
>         edid = drm_do_get_edid(connector, it66121_get_edid_block, ctx);
> +
> +out_unlock:
>         mutex_unlock(&ctx->lock);
>
>         return edid;
> --
> 2.35.1
>

Reviewed-by: Robert Foss <robert.foss@linaro.org>

Re: [PATCH 09/10] drm: bridge: it66121: Move VID/PID to new it66121_chip_info structure

[Robert Foss]

On Wed, 14 Dec 2022 at 14:01, Paul Cercueil <paul@crapouillou.net> wrote:
>
> This will make it easier later to introduce support for new chips in
> this driver.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/gpu/drm/bridge/ite-it66121.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index 7972003d4776..43b027b85b8e 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -35,10 +35,6 @@
>  #define IT66121_DEVICE_ID0_REG                 0x02
>  #define IT66121_DEVICE_ID1_REG                 0x03
>
> -#define IT66121_VENDOR_ID0                     0x54
> -#define IT66121_VENDOR_ID1                     0x49
> -#define IT66121_DEVICE_ID0                     0x12
> -#define IT66121_DEVICE_ID1                     0x06
>  #define IT66121_REVISION_MASK                  GENMASK(7, 4)
>  #define IT66121_DEVICE_ID1_MASK                        GENMASK(3, 0)
>
> @@ -286,13 +282,12 @@
>  #define IT66121_AUD_SWL_16BIT                  0x2
>  #define IT66121_AUD_SWL_NOT_INDICATED          0x0
>
> -#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_AFE_CLK_HIGH                   80000 /* Khz */
>
> +struct it66121_chip_info {
> +       u16 vid, pid;
> +};
> +
>  struct it66121_ctx {
>         struct regmap *regmap;
>         struct drm_bridge bridge;
> @@ -311,6 +306,7 @@ struct it66121_ctx {
>                 u8 swl;
>                 bool auto_cts;
>         } audio;
> +       const struct it66121_chip_info *info;
>  };
>
>  static const struct regmap_range_cfg it66121_regmap_banks[] = {
> @@ -1451,6 +1447,7 @@ static const char * const it66121_supplies[] = {
>
>  static int it66121_probe(struct i2c_client *client)
>  {
> +       const struct i2c_device_id *id = i2c_client_get_device_id(client);
>         u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 };
>         struct device_node *ep;
>         int ret;
> @@ -1472,6 +1469,7 @@ static int it66121_probe(struct i2c_client *client)
>
>         ctx->dev = dev;
>         ctx->client = client;
> +       ctx->info = (const struct it66121_chip_info *) id->driver_data;
>
>         of_property_read_u32(ep, "bus-width", &ctx->bus_width);
>         of_node_put(ep);
> @@ -1523,8 +1521,8 @@ static int it66121_probe(struct i2c_client *client)
>         revision_id = FIELD_GET(IT66121_REVISION_MASK, device_ids[1]);
>         device_ids[1] &= IT66121_DEVICE_ID1_MASK;
>
> -       if (vendor_ids[0] != IT66121_VENDOR_ID0 || vendor_ids[1] != IT66121_VENDOR_ID1 ||
> -           device_ids[0] != IT66121_DEVICE_ID0 || device_ids[1] != IT66121_DEVICE_ID1) {
> +       if ((vendor_ids[1] << 8 | vendor_ids[0]) != ctx->info->vid ||
> +           (device_ids[1] << 8 | device_ids[0]) != ctx->info->pid) {
>                 return -ENODEV;
>         }
>
> @@ -1563,8 +1561,13 @@ static const struct of_device_id it66121_dt_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, it66121_dt_match);
>
> +static const struct it66121_chip_info it66121_chip_info = {
> +       .vid = 0x4954,
> +       .pid = 0x0612,
> +};
> +
>  static const struct i2c_device_id it66121_id[] = {
> -       { "it66121", 0 },
> +       { "it66121", (kernel_ulong_t) &it66121_chip_info },
>         { }
>  };
>  MODULE_DEVICE_TABLE(i2c, it66121_id);
> --
> 2.35.1
>

Reviewed-by: Robert Foss <robert.foss@linaro.org>

Re: [PATCH 10/10] drm: bridge: it66121: Add support for the IT6610

[Robert Foss]

On Wed, 14 Dec 2022 at 14:01, Paul Cercueil <paul@crapouillou.net> wrote:
>
> Add support for the IT6610 HDMI encoder.
>
> The hardware is very similar, and therefore the driver did not require
> too many changes. Some bits are only available on the IT66121, and
> vice-versa. Also, the IT6610 requires specific polarities on the DE and
> pixel lines.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/gpu/drm/bridge/ite-it66121.c | 108 +++++++++++++++++++++------
>  1 file changed, 86 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index 43b027b85b8e..b34860871627 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -68,6 +68,7 @@
>  #define IT66121_AFE_XP_ENO                     BIT(4)
>  #define IT66121_AFE_XP_RESETB                  BIT(3)
>  #define IT66121_AFE_XP_PWDI                    BIT(2)
> +#define IT6610_AFE_XP_BYPASS                   BIT(0)
>
>  #define IT66121_AFE_IP_REG                     0x64
>  #define IT66121_AFE_IP_GAINBIT                 BIT(7)
> @@ -284,7 +285,13 @@
>
>  #define IT66121_AFE_CLK_HIGH                   80000 /* Khz */
>
> +enum chip_id {
> +       ID_IT6610,
> +       ID_IT66121,
> +};
> +
>  struct it66121_chip_info {
> +       enum chip_id id;
>         u16 vid, pid;
>  };
>
> @@ -391,16 +398,22 @@ static int it66121_configure_afe(struct it66121_ctx *ctx,
>
>                 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_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;
> +               if (ctx->info->id == ID_IT66121) {
> +                       ret = regmap_write_bits(ctx->regmap, IT66121_AFE_IP_REG,
> +                                               IT66121_AFE_IP_EC1, 0);
> +                       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 |
> @@ -411,17 +424,24 @@ static int it66121_configure_afe(struct it66121_ctx *ctx,
>
>                 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);
> +                                       IT66121_AFE_IP_ER0,
> +                                       IT66121_AFE_IP_ER0);
>                 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;
> +               if (ctx->info->id == ID_IT66121) {
> +                       ret = regmap_write_bits(ctx->regmap, IT66121_AFE_IP_REG,
> +                                               IT66121_AFE_IP_EC1,
> +                                               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 */
> @@ -430,6 +450,14 @@ static int it66121_configure_afe(struct it66121_ctx *ctx,
>         if (ret)
>                 return ret;
>
> +       if (ctx->info->id == ID_IT6610) {
> +               ret = regmap_write_bits(ctx->regmap, IT66121_AFE_XP_REG,
> +                                       IT6610_AFE_XP_BYPASS,
> +                                       IT6610_AFE_XP_BYPASS);
> +               if (ret)
> +                       return ret;
> +       }
> +
>         return it66121_fire_afe(ctx);
>  }
>
> @@ -491,7 +519,6 @@ 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;
> @@ -572,10 +599,12 @@ static int it66121_bridge_attach(struct drm_bridge *bridge,
>         if (ret)
>                 return ret;
>
> -       ret = regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
> -                               IT66121_CLK_BANK_PWROFF_RCLK, 0);
> -       if (ret)
> -               return ret;
> +       if (ctx->info->id == ID_IT66121) {
> +               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);
> @@ -713,6 +742,24 @@ static void it66121_bridge_disable(struct drm_bridge *bridge,
>         ctx->connector = NULL;
>  }
>
> +static int it66121_bridge_check(struct drm_bridge *bridge,
> +                               struct drm_bridge_state *bridge_state,
> +                               struct drm_crtc_state *crtc_state,
> +                               struct drm_connector_state *conn_state)
> +{
> +       struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
> +
> +       if (ctx->info->id == ID_IT6610) {
> +               /* The IT6610 only supports these settings */
> +               bridge_state->input_bus_cfg.flags |= DRM_BUS_FLAG_DE_HIGH |
> +                       DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE;
> +               bridge_state->input_bus_cfg.flags &=
> +                       ~DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE;
> +       }
> +
> +       return 0;
> +}
> +
>  static
>  void it66121_bridge_mode_set(struct drm_bridge *bridge,
>                              const struct drm_display_mode *mode,
> @@ -758,9 +805,12 @@ void it66121_bridge_mode_set(struct drm_bridge *bridge,
>         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))
> +       if (ctx->info->id == ID_IT66121 &&
> +           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;
> @@ -768,7 +818,11 @@ void it66121_bridge_mode_set(struct drm_bridge *bridge,
>         if (it66121_configure_afe(ctx, adjusted_mode))
>                 goto unlock;
>
> -       regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG, IT66121_CLK_BANK_PWROFF_TXCLK, 0);
> +       if (ctx->info->id == ID_IT66121 &&
> +           regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
> +                             IT66121_CLK_BANK_PWROFF_TXCLK, 0)) {
> +               goto unlock;
> +       }
>
>  unlock:
>         mutex_unlock(&ctx->lock);
> @@ -859,6 +913,7 @@ static const struct drm_bridge_funcs it66121_bridge_funcs = {
>         .atomic_get_input_bus_fmts = it66121_bridge_atomic_get_input_bus_fmts,
>         .atomic_enable = it66121_bridge_enable,
>         .atomic_disable = it66121_bridge_disable,
> +       .atomic_check = it66121_bridge_check,
>         .mode_set = it66121_bridge_mode_set,
>         .mode_valid = it66121_bridge_mode_valid,
>         .detect = it66121_bridge_detect,
> @@ -1557,17 +1612,26 @@ static void it66121_remove(struct i2c_client *client)
>
>  static const struct of_device_id it66121_dt_match[] = {
>         { .compatible = "ite,it66121" },
> +       { .compatible = "ite,it6610" },
>         { }
>  };
>  MODULE_DEVICE_TABLE(of, it66121_dt_match);
>
>  static const struct it66121_chip_info it66121_chip_info = {
> +       .id = ID_IT66121,
>         .vid = 0x4954,
>         .pid = 0x0612,
>  };
>
> +static const struct it66121_chip_info it6610_chip_info = {
> +       .id = ID_IT6610,
> +       .vid = 0xca00,
> +       .pid = 0x0611,
> +};
> +
>  static const struct i2c_device_id it66121_id[] = {
>         { "it66121", (kernel_ulong_t) &it66121_chip_info },
> +       { "it6610", (kernel_ulong_t) &it6610_chip_info },
>         { }
>  };
>  MODULE_DEVICE_TABLE(i2c, it66121_id);
> --
> 2.35.1
>

Reviewed-by: Robert Foss <robert.foss@linaro.org>

Re: [PATCH 01/10] dt-bindings: display: bridge: it66121: Add compatible string for IT6610

[Paul Cercueil]

Le jeudi 15 décembre 2022 à 11:55 +0100, Robert Foss a écrit :
> On Wed, 14 Dec 2022 at 13:58, Paul Cercueil <paul@crapouillou.net>
> wrote:
> > 
> > Add a new ite,it6610 compatible string to the IT66121 binding
> > documentation, since the two chips are very similar.
> > 
> > Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > ---
> >  .../devicetree/bindings/display/bridge/ite,it66121.yaml       | 4
> > +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> > b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> > index 1b2185be92cd..72957be0ba3c 100644
> > ---
> > a/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> > +++
> > b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> > @@ -17,7 +17,9 @@ description: |
> > 
> >  properties:
> >    compatible:
> > -    const: ite,it66121
> > +    enum:
> > +      - ite,it66121
> > +      - ite,it6610
> > 
> >    reg:
> >      maxItems: 1
> > --
> > 2.35.1
> > 
> 
> Reviewed-by: Robert Foss <robert.foss@linaro.org>

Series applied to drm-misc-next.

Thanks for the reviews!

Cheers,
-Paul

Re: [PATCH 01/10] dt-bindings: display: bridge: it66121: Add compatible string for IT6610

[Krzysztof Kozlowski]

On 16/12/2022 11:46, Paul Cercueil wrote:

>>>  properties:
>>>    compatible:
>>> -    const: ite,it66121
>>> +    enum:
>>> +      - ite,it66121
>>> +      - ite,it6610

These should be ordered alphabetically. What's with the tendency of
adding always to the end?

>>>
>>>    reg:
>>>      maxItems: 1
>>> --
>>> 2.35.1
>>>
>>
>> Reviewed-by: Robert Foss <robert.foss@linaro.org>
> 
> Series applied to drm-misc-next.
> 
I wished you give DT maintainers a bit more time than two days.

Best regards,
Krzysztof

Re: [PATCH 01/10] dt-bindings: display: bridge: it66121: Add compatible string for IT6610

[Paul Cercueil]

Hi Krzysztof,

Le vendredi 16 décembre 2022 à 12:21 +0100, Krzysztof Kozlowski a
écrit :
> On 16/12/2022 11:46, Paul Cercueil wrote:
> 
> > > >  properties:
> > > >    compatible:
> > > > -    const: ite,it66121
> > > > +    enum:
> > > > +      - ite,it66121
> > > > +      - ite,it6610
> 
> These should be ordered alphabetically. What's with the tendency of
> adding always to the end?

I'm too used to the "inverse christmas tree" sort :)

I can send a quickfix patch if you really want alphabetical order.

> > > > 
> > > >    reg:
> > > >      maxItems: 1
> > > > --
> > > > 2.35.1
> > > > 
> > > 
> > > Reviewed-by: Robert Foss <robert.foss@linaro.org>
> > 
> > Series applied to drm-misc-next.
> > 
> I wished you give DT maintainers a bit more time than two days.

Noted. Sorry about that.

Cheers,
-Paul

Re: [PATCH 01/10] dt-bindings: display: bridge: it66121: Add compatible string for IT6610

[Krzysztof Kozlowski]

On 16/12/2022 13:21, Paul Cercueil wrote:
> Hi Krzysztof,
> 
> Le vendredi 16 décembre 2022 à 12:21 +0100, Krzysztof Kozlowski a
> écrit :
>> On 16/12/2022 11:46, Paul Cercueil wrote:
>>
>>>>>  properties:
>>>>>    compatible:
>>>>> -    const: ite,it66121
>>>>> +    enum:
>>>>> +      - ite,it66121
>>>>> +      - ite,it6610
>>
>> These should be ordered alphabetically. What's with the tendency of
>> adding always to the end?
> 
> I'm too used to the "inverse christmas tree" sort :)

Since these are not variables and they will not get shorter, any
christmas tree sorting here is the same conflict-prone as adding to the end.

> 
> I can send a quickfix patch if you really want alphabetical order.

No, no need.


Best regards,
Krzysztof

Re: [PATCH 01/10] dt-bindings: display: bridge: it66121: Add compatible string for IT6610

[Rob Herring]

On Fri, Dec 16, 2022 at 01:21:54PM +0100, Paul Cercueil wrote:
> Hi Krzysztof,
> 
> Le vendredi 16 décembre 2022 à 12:21 +0100, Krzysztof Kozlowski a
> écrit :
> > On 16/12/2022 11:46, Paul Cercueil wrote:
> > 
> > > > >  properties:
> > > > >    compatible:
> > > > > -    const: ite,it66121
> > > > > +    enum:
> > > > > +      - ite,it66121
> > > > > +      - ite,it6610
> > 
> > These should be ordered alphabetically. What's with the tendency of
> > adding always to the end?
> 
> I'm too used to the "inverse christmas tree" sort :)

Come on, the DT standard is sideways christmas tree. ;)

Rob