<pre>
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 <granquet@baylibre.com>
> ---
>

[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;
> +}
> +
>

</pre><!--type:text--><!--{--><pre>************* MEDIATEK Confidentiality Notice ********************
The information contained in this e-mail message (including any 
attachments) may be confidential, proprietary, privileged, or otherwise
exempt from disclosure under applicable laws. It is intended to be 
conveyed only to the designated recipient(s). Any use, dissemination, 
distribution, printing, retaining or copying of this e-mail (including its 
attachments) by unintended recipient(s) is strictly prohibited and may 
be unlawful. If you are not an intended recipient of this e-mail, or believe 
that you have received this e-mail in error, please notify the sender 
immediately (by replying to this e-mail), delete any and all copies of 
this e-mail (including any attachments) from your system, and do not
disclose the content of this e-mail to any other person. Thank you!
</pre><!--}-->