[PATCH v4 5/8] drm/mediatek: hdmi: add v2 support

Tue Jun 13 05:35:45 UTC 2023

An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230613/b1cbd2e3/attachment-0001.htm>
-------------- next part --------------
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 at 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;
> +}
> +
> 