Hi, Guillaume:

On Mon, 2023-05-29 at 16:31 +0200, Guillaume Ranquet wrote:

 	 
External email : Please do not click links or open attachments until
you have verified the sender or the content.
 Adds hdmi and hdmi-ddc support for v2 IP.

Signed-off-by: Guillaume Ranquet <redacted>
---
 

[snip]

+
+static unsigned char
+ddcm_read_hdmi(struct mtk_hdmi_ddc *ddc,
+       unsigned int u4_clk_div, unsigned char uc_dev, unsigned int
u4_addr,
+       enum sif_bit_t_hdmi uc_addr_type, unsigned char *puc_value,
+       unsigned int u4_count)
+{
+u32 val;
+unsigned int i, temp_length, loop_counter, uc_read_count, uc_idx =
0;

I would like the variable naming as mtk_hdmi_ddc_read_msg() so that
it's easy to compare this two driver. So does the variable calculating
way.

+
+if (!puc_value || !u4_count || !u4_clk_div)
+return 0;
+
+uc_idx = 0;
+regmap_read(ddc->regs, HDCP2X_DDCM_STATUS, &val);
+if (val & DDC_I2C_BUS_LOW) {
+regmap_update_bits(ddc->regs, DDC_CTRL, FIELD_PREP(DDC_CMD,
CLOCK_SCL), DDC_CMD);
+usleep_range(250, 300);
+}
+
+regmap_update_bits(ddc->regs, DDC_CTRL, FIELD_PREP(DDC_CMD,
CLEAR_FIFO), DDC_CMD);
+
+if (u4_count >= 16) {
+temp_length = 16;
+loop_counter =
+u4_count / 16 + (u4_count % 16 == 0);
+} else {
+temp_length = u4_count;
+loop_counter = 1;
+}
+if (uc_dev >= EDID_ID) {
+if (u4_clk_div < DDC2_CLOK_EDID)
+u4_clk_div = DDC2_CLOK_EDID;
+}
+regmap_update_bits(ddc->regs, HPD_DDC_CTRL,
FIELD_PREP(DDC_DELAY_CNT, u4_clk_div), DDC_DELAY_CNT);
+for (i = 0; i < loop_counter; i++) {
+if (i == (loop_counter - 1) && i != 0 &&
+u4_count % 16)
+temp_length = u4_count % 16;
+if (uc_dev > EDID_ID) {
+regmap_update_bits(ddc->regs, SCDC_CTRL, FIELD_PREP(DDC_SEGMENT,
uc_dev - EDID_ID), DDC_SEGMENT);
+regmap_write(ddc->regs, DDC_CTRL, FIELD_PREP(DDC_CMD,
ENH_READ_NO_ACK) |
+  FIELD_PREP(DDC_DIN_CNT, temp_length) |
+  FIELD_PREP(DDC_OFFSET, u4_addr + i * temp_length) |
+  (EDID_ID << 1));
+} else {
+regmap_write(ddc->regs, DDC_CTRL, FIELD_PREP(DDC_CMD,
SEQ_READ_NO_ACK) |
+  FIELD_PREP(DDC_DIN_CNT, temp_length) |
+  FIELD_PREP(DDC_OFFSET, u4_addr + i * 16) |
+  (uc_dev << 1));
+}
+usleep_range(5000, 5500);
+if (regmap_read_poll_timeout(ddc->regs, HPD_DDC_STATUS, val,
+!(val & DDC_I2C_IN_PROG), 2000, temp_length + 5)) {
+dev_err(ddc->dev, "time out waiting for DDC I2C\n");
+return 0;
+}
+regmap_read(ddc->regs, HDCP2X_DDCM_STATUS, &val);
+if ((val & (DDC_I2C_NO_ACK | DDC_I2C_BUS_LOW))) {
+regmap_read(ddc->regs, HDCP2X_DDCM_STATUS, &val);
+if (val & DDC_I2C_BUS_LOW) {
+regmap_update_bits(ddc->regs, DDC_CTRL,
+ FIELD_PREP(DDC_CMD, CLOCK_SCL), DDC_CMD);
+usleep_range(250, 300);
+}
+return 0;
+}
+for (uc_idx = 0; uc_idx < temp_length; uc_idx++) {
+regmap_write(ddc->regs, SI2C_CTRL,
+  FIELD_PREP(SI2C_ADDR, SI2C_ADDR_READ) | SI2C_RD);
+regmap_write(ddc->regs, SI2C_CTRL,
+  FIELD_PREP(SI2C_ADDR, SI2C_ADDR_READ) | SI2C_CONFIRM_READ);
+regmap_read(ddc->regs, HPD_DDC_STATUS, &val);
+puc_value[i * 16 + uc_idx] = FIELD_GET(DDC_DATA_OUT, val);
+/*
+ * when reading edid, if hdmi module been reset,
+ * ddc will fail and it's speed will be set to 400.
+ */
+regmap_read(ddc->regs, HPD_DDC_CTRL, &val);
+if (((val >> 16) & 0xFFFF) < DDC2_CLOK)
+return 0;
+
+uc_read_count = i * 16 + uc_idx + 1;
+}
+}
+return uc_read_count;

uc_read_count is u4_count, isn't it? So you just return u4_count or 0,
so I would like to return 0 for success or error code.

+}
+
+static unsigned char vddc_read(struct mtk_hdmi_ddc *ddc, unsigned
int u4_clk_div,
+       unsigned char uc_dev, unsigned int u4_addr,
+       enum sif_bit_t_hdmi uc_addr_type,
+       unsigned char *puc_value, unsigned int u4_count)
+{
+unsigned int u4_read_count = 0;
+unsigned char uc_return_value = 0;
+
+if (!puc_value || !u4_count || !u4_clk_div ||
+    uc_addr_type > SIF_16_BIT_HDMI ||
+    (uc_addr_type == SIF_8_BIT_HDMI && u4_addr > 255) ||
+    (uc_addr_type == SIF_16_BIT_HDMI && u4_addr > 65535)) {
+return 0;
+}
+
+if (uc_addr_type == SIF_8_BIT_HDMI)
+u4_read_count = 255 - u4_addr + 1;
+else if (uc_addr_type == SIF_16_BIT_HDMI)
+u4_read_count = 65535 - u4_addr + 1;
+
+u4_read_count = (u4_read_count > u4_count) ? u4_count :
u4_read_count;

If u4_read_count < u4_count, the return value would not be u4_count,
and the checking in mtk_hdmi_ddc_xfer() would fail. I would it's not
necessary to do the next statement, just return 0 here.
I'm curious about why do this? Why not read u4_count?

Regards,
CK

+uc_return_value = ddcm_read_hdmi(ddc, u4_clk_div, uc_dev, u4_addr,
+ uc_addr_type, puc_value, u4_read_count);
+return uc_return_value;
+}
+
+static bool fg_ddc_data_read(struct mtk_hdmi_ddc *ddc,
+      unsigned char b_dev,
+      unsigned char b_data_addr,
+      unsigned char b_data_count,
+      unsigned char *pr_data)
+{
+int ret;
+
+mutex_lock(&ddc->mtx);
+
+hdmi_ddc_request(ddc);
+ret = vddc_read(ddc, DDC2_CLOK, b_dev, b_data_addr, SIF_8_BIT_HDMI,
+      pr_data, b_data_count);
+mutex_unlock(&ddc->mtx);
+
+return ret == b_data_count;
+}
+
+static void fg_ddc_data_write(struct mtk_hdmi_ddc *ddc,
+       unsigned char b_dev,
+       unsigned char b_data_addr,
+       unsigned char b_data_count,
+       unsigned char *pr_data)
+{
+unsigned int i;
+
+mutex_lock(&ddc->mtx);
+
+hdmi_ddc_request(ddc);
+for (i = 0; i < b_data_count; i++)
+mtk_ddc_wr_one(ddc, b_dev, b_data_addr + i, *(pr_data + i));
+
+mutex_unlock(&ddc->mtx);
+}
+
+static int mtk_hdmi_ddc_xfer(struct i2c_adapter *adapter, struct
i2c_msg *msgs,
+     int num)
+{
+struct mtk_hdmi_ddc *ddc = adapter->algo_data;
+struct device *dev = adapter->dev.parent;
+bool ret;
+int i;
+unsigned char offset;
+
+if (!ddc)
+return -EINVAL;
+
+for (i = 0; i < num; i++) {
+struct i2c_msg *msg = &msgs[i];
+
+if (msg->flags & I2C_M_RD) {
+/* The underlying DDC hardware always issue a write request
+ * that assigns the read offset as part of the read operation.
+ * Therefore we need to use the offset value assigned
+ * in the previous write request from the drm_edid.c
+ */
+ret = fg_ddc_data_read(ddc, msg->addr,
+       offset, /* determined by previous write requests */
+       (msg->len), &msg->buf[0]);
+if (!ret) {
+dev_err(dev, "ddc read failed : %d\n", ret);
+return ret;
+}
+} else {
+fg_ddc_data_write(ddc, msg->addr, msg->buf[0],
+(msg->len - 1), &msg->buf[1]);
+
+/* we store the offset value requested by drm_edid framework
+ * to use in subsequent read requests.
+ */
+if (DDC_ADDR == msg->addr && 1 == msg->len)
+offset = msg->buf[0];
+}
+}
+
+return i;
+}
+

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel