Re: [PATCH v4] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus

[Jonathan Liu]

Hi Maxime,

On 27 June 2017 at 05:05, Maxime Ripard
<maxime.ripard@xxxxxxxxxxxxxxxxxx> wrote:
> On Sat, Jun 24, 2017 at 04:10:54PM +1000, Jonathan Liu wrote:
>> The documentation for drm_do_get_edid in drivers/gpu/drm/drm_edid.c states:
>> "As in the general case the DDC bus is accessible by the kernel at the I2C
>> level, drivers must make all reasonable efforts to expose it as an I2C
>> adapter and use drm_get_edid() instead of abusing this function."
>>
>> Exposing the DDC bus as an I2C adapter is more beneficial as it can be used
>> for purposes other than reading the EDID such as modifying the EDID or
>> using the HDMI DDC pins as an I2C bus through the I2C dev interface from
>> userspace (e.g. i2c-tools).
>>
>> Implement this for A10s.
>>
>> Signed-off-by: Jonathan Liu <net147@xxxxxxxxx>
>> ---
>> Changes for v4:
>>  - Carry over copyright from initial I2C code into sun4i_hdmi_i2c.c
>>  - Clean up indentation in sun4i_hdmi.h
>>  - Rename SUN4I_HDMI_DDC_MAX_TRANSFER_SIZE to SUN4I_HDMI_DDC_BYTE_COUNT_MAX
>>    and group it under the SUN4I_HDMI_DDC_BYTE_COUNT_REG define, changing the
>>    value to use the GENMASK macro to make it clear that it is derived from
>>    the width of the field in the register
>>  - Fix SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_UNDERFLOW typo which should be
>>    SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW
>>  - Remove redundant rewriting of SUN4I_HDMI_DDC_INT_STATUS_REG register
>>  - Change struct i2c_adapter to be const by using devm_kmemdup on creation
>>  - Return -ETIMEDOUT instead of -EIO if there is timeout while transferring an
>>    I2C message
>>  - Instead of waiting for 1-2 bytes to transfer, wait for the time it would
>>    take for remaining bytes to transfer (limited by FIFO size)
>>  - Add additional comments
>>
>> Changes for v3:
>>  - Explain why drm_do_get_edid should be used and why it's better to expose it
>>    as an I2C adapter in commit message
>>  - Reorder bit defines in descending order for consistency
>>  - Keep old unused macros instead of removing them
>>  - The v2 algorithm split large transfers into 16 byte transfers but this may
>>    cause a large single write to be treated as multiple writes causing data
>>    corruption. The algorithm has been reworked to not split larger transfers
>>    and make more use of the FIFO to avoid this.
>>  - Moved the creation of the DDC clock from sun4i_hdmi_enc.c to
>>    sun4i_hdmi_i2c.c
>>  - Reformatted code
>>  - Instead of masking bits that we don't want to check for errors, explicitly
>>    check the error bits
>>  - Clear error bits at start of transfer in case of error from previous transfer
>>  - Poll for completion of FIFO clear after setting FIFO clear bit
>>
>> Changes for v2:
>>  - Rebased against Maxime's sunxi-drm/for-next branch
>>  - Fix up error paths in sun4i_hdmi_bind so that the I2C adapter is deleted if
>>    any of the calls after the I2C adapter is created fails
>>  - Remove unnecessary includes in sun4i_hdmi_i2c.c
>>
>>  drivers/gpu/drm/sun4i/Makefile         |   1 +
>>  drivers/gpu/drm/sun4i/sun4i_hdmi.h     |  23 ++++
>>  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 101 ++------------
>>  drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 242 +++++++++++++++++++++++++++++++++
>>  4 files changed, 277 insertions(+), 90 deletions(-)
>>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c
>>
>> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
>> index e29fd3a2ba9c..43c753cafc88 100644
>> --- a/drivers/gpu/drm/sun4i/Makefile
>> +++ b/drivers/gpu/drm/sun4i/Makefile
>> @@ -2,6 +2,7 @@ sun4i-drm-y += sun4i_drv.o
>>  sun4i-drm-y += sun4i_framebuffer.o
>>
>>  sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
>> +sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
>>  sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
>>  sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o
>>
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> index 2f2f2ff1ea63..eaff92fe236c 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> @@ -96,6 +96,7 @@
>>  #define SUN4I_HDMI_DDC_CTRL_ENABLE           BIT(31)
>>  #define SUN4I_HDMI_DDC_CTRL_START_CMD                BIT(30)
>>  #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK    BIT(8)
>> +#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE   (1 << 8)
>>  #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ    (0 << 8)
>>  #define SUN4I_HDMI_DDC_CTRL_RESET            BIT(0)
>>
>> @@ -105,14 +106,33 @@
>>  #define SUN4I_HDMI_DDC_ADDR_OFFSET(off)              (((off) & 0xff) << 8)
>>  #define SUN4I_HDMI_DDC_ADDR_SLAVE(addr)              ((addr) & 0xff)
>>
>> +#define SUN4I_HDMI_DDC_INT_STATUS_REG                0x50c
>> +#define SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION     BIT(7)
>> +#define SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOW              BIT(6)
>> +#define SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW               BIT(5)
>> +#define SUN4I_HDMI_DDC_INT_STATUS_FIFO_REQUEST                       BIT(4)
>> +#define SUN4I_HDMI_DDC_INT_STATUS_ARBITRATION_ERROR          BIT(3)
>> +#define SUN4I_HDMI_DDC_INT_STATUS_ACK_ERROR                  BIT(2)
>> +#define SUN4I_HDMI_DDC_INT_STATUS_BUS_ERROR                  BIT(1)
>> +#define SUN4I_HDMI_DDC_INT_STATUS_TRANSFER_COMPLETE          BIT(0)
>> +
>>  #define SUN4I_HDMI_DDC_FIFO_CTRL_REG 0x510
>>  #define SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR               BIT(31)
>>
>> +#define SUN4I_HDMI_DDC_FIFO_STATUS_REG       0x514
>> +#define SUN4I_HDMI_DDC_FIFO_STATUS_FULL              BIT(6)
>> +#define SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY     BIT(5)
>> +#define SUN4I_HDMI_DDC_FIFO_STATUS_LEVEL_MASK        GENMASK(4, 0)
>> +
>>  #define SUN4I_HDMI_DDC_FIFO_DATA_REG 0x518
>> +
>>  #define SUN4I_HDMI_DDC_BYTE_COUNT_REG        0x51c
>> +#define SUN4I_HDMI_DDC_BYTE_COUNT_MAX                GENMASK(9, 0)
>>
>>  #define SUN4I_HDMI_DDC_CMD_REG               0x520
>>  #define SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ        6
>> +#define SUN4I_HDMI_DDC_CMD_IMPLICIT_READ     5
>> +#define SUN4I_HDMI_DDC_CMD_IMPLICIT_WRITE    3
>>
>>  #define SUN4I_HDMI_DDC_CLK_REG               0x528
>>  #define SUN4I_HDMI_DDC_CLK_M(m)                      (((m) & 0x7) << 3)
>> @@ -146,6 +166,8 @@ struct sun4i_hdmi {
>>       struct clk              *ddc_clk;
>>       struct clk              *tmds_clk;
>>
>> +     struct i2c_adapter      *i2c;
>> +
>>       struct sun4i_drv        *drv;
>>
>>       bool                    hdmi_monitor;
>> @@ -153,5 +175,6 @@ struct sun4i_hdmi {
>>
>>  int sun4i_ddc_create(struct sun4i_hdmi *hdmi, struct clk *clk);
>>  int sun4i_tmds_create(struct sun4i_hdmi *hdmi);
>> +int sun4i_hdmi_i2c_create(struct device *dev, struct sun4i_hdmi *hdmi);
>>
>>  #endif /* _SUN4I_HDMI_H_ */
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> index d3398f6250ef..b74607feb35c 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> @@ -29,8 +29,6 @@
>>  #include "sun4i_hdmi.h"
>>  #include "sun4i_tcon.h"
>>
>> -#define DDC_SEGMENT_ADDR     0x30
>> -
>>  static inline struct sun4i_hdmi *
>>  drm_encoder_to_sun4i_hdmi(struct drm_encoder *encoder)
>>  {
>> @@ -184,93 +182,13 @@ static const struct drm_encoder_funcs sun4i_hdmi_funcs = {
>>       .destroy        = drm_encoder_cleanup,
>>  };
>>
>> -static int sun4i_hdmi_read_sub_block(struct sun4i_hdmi *hdmi,
>> -                                  unsigned int blk, unsigned int offset,
>> -                                  u8 *buf, unsigned int count)
>> -{
>> -     unsigned long reg;
>> -     int i;
>> -
>> -     reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
>> -     reg &= ~SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK;
>> -     writel(reg | SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ,
>> -            hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
>> -
>> -     writel(SUN4I_HDMI_DDC_ADDR_SEGMENT(offset >> 8) |
>> -            SUN4I_HDMI_DDC_ADDR_EDDC(DDC_SEGMENT_ADDR << 1) |
>> -            SUN4I_HDMI_DDC_ADDR_OFFSET(offset) |
>> -            SUN4I_HDMI_DDC_ADDR_SLAVE(DDC_ADDR),
>> -            hdmi->base + SUN4I_HDMI_DDC_ADDR_REG);
>> -
>> -     reg = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG);
>> -     writel(reg | SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR,
>> -            hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG);
>> -
>> -     writel(count, hdmi->base + SUN4I_HDMI_DDC_BYTE_COUNT_REG);
>> -     writel(SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ,
>> -            hdmi->base + SUN4I_HDMI_DDC_CMD_REG);
>> -
>> -     reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
>> -     writel(reg | SUN4I_HDMI_DDC_CTRL_START_CMD,
>> -            hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
>> -
>> -     if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG, reg,
>> -                            !(reg & SUN4I_HDMI_DDC_CTRL_START_CMD),
>> -                            100, 100000))
>> -             return -EIO;
>> -
>> -     for (i = 0; i < count; i++)
>> -             buf[i] = readb(hdmi->base + SUN4I_HDMI_DDC_FIFO_DATA_REG);
>> -
>> -     return 0;
>> -}
>> -
>> -static int sun4i_hdmi_read_edid_block(void *data, u8 *buf, unsigned int blk,
>> -                                   size_t length)
>> -{
>> -     struct sun4i_hdmi *hdmi = data;
>> -     int retry = 2, i;
>> -
>> -     do {
>> -             for (i = 0; i < length; i += SUN4I_HDMI_DDC_FIFO_SIZE) {
>> -                     unsigned char offset = blk * EDID_LENGTH + i;
>> -                     unsigned int count = min((unsigned int)SUN4I_HDMI_DDC_FIFO_SIZE,
>> -                                              length - i);
>> -                     int ret;
>> -
>> -                     ret = sun4i_hdmi_read_sub_block(hdmi, blk, offset,
>> -                                                     buf + i, count);
>> -                     if (ret)
>> -                             return ret;
>> -             }
>> -     } while (!drm_edid_block_valid(buf, blk, true, NULL) && (retry--));
>> -
>> -     return 0;
>> -}
>> -
>>  static int sun4i_hdmi_get_modes(struct drm_connector *connector)
>>  {
>>       struct sun4i_hdmi *hdmi = drm_connector_to_sun4i_hdmi(connector);
>> -     unsigned long reg;
>>       struct edid *edid;
>>       int ret;
>>
>> -     /* Reset i2c controller */
>> -     writel(SUN4I_HDMI_DDC_CTRL_ENABLE | SUN4I_HDMI_DDC_CTRL_RESET,
>> -            hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
>> -     if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG, reg,
>> -                            !(reg & SUN4I_HDMI_DDC_CTRL_RESET),
>> -                            100, 2000))
>> -             return -EIO;
>> -
>> -     writel(SUN4I_HDMI_DDC_LINE_CTRL_SDA_ENABLE |
>> -            SUN4I_HDMI_DDC_LINE_CTRL_SCL_ENABLE,
>> -            hdmi->base + SUN4I_HDMI_DDC_LINE_CTRL_REG);
>> -
>> -     clk_prepare_enable(hdmi->ddc_clk);
>> -     clk_set_rate(hdmi->ddc_clk, 100000);
>> -
>> -     edid = drm_do_get_edid(connector, sun4i_hdmi_read_edid_block, hdmi);
>> +     edid = drm_get_edid(connector, hdmi->i2c);
>>       if (!edid)
>>               return 0;
>>
>> @@ -282,8 +200,6 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
>>       ret = drm_add_edid_modes(connector, edid);
>>       kfree(edid);
>>
>> -     clk_disable_unprepare(hdmi->ddc_clk);
>> -
>>       return ret;
>>  }
>>
>> @@ -407,9 +323,9 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
>>               SUN4I_HDMI_PLL_CTRL_PLL_EN;
>>       writel(reg, hdmi->base + SUN4I_HDMI_PLL_CTRL_REG);
>>
>> -     ret = sun4i_ddc_create(hdmi, hdmi->tmds_clk);
>> +     ret = sun4i_hdmi_i2c_create(dev, hdmi);
>>       if (ret) {
>> -             dev_err(dev, "Couldn't create the DDC clock\n");
>> +             dev_err(dev, "Couldn't create the HDMI I2C adapter\n");
>>               return ret;
>>       }
>>
>> @@ -422,13 +338,15 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
>>                              NULL);
>>       if (ret) {
>>               dev_err(dev, "Couldn't initialise the HDMI encoder\n");
>> -             return ret;
>> +             goto err_del_i2c_adapter;
>>       }
>>
>>       hdmi->encoder.possible_crtcs = drm_of_find_possible_crtcs(drm,
>>                                                                 dev->of_node);
>> -     if (!hdmi->encoder.possible_crtcs)
>> -             return -EPROBE_DEFER;
>> +     if (!hdmi->encoder.possible_crtcs) {
>> +             ret = -EPROBE_DEFER;
>> +             goto err_del_i2c_adapter;
>> +     }
>>
>>       drm_connector_helper_add(&hdmi->connector,
>>                                &sun4i_hdmi_connector_helper_funcs);
>> @@ -451,6 +369,8 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
>>
>>  err_cleanup_connector:
>>       drm_encoder_cleanup(&hdmi->encoder);
>> +err_del_i2c_adapter:
>> +     i2c_del_adapter(hdmi->i2c);
>>       return ret;
>>  }
>>
>> @@ -461,6 +381,7 @@ static void sun4i_hdmi_unbind(struct device *dev, struct device *master,
>>
>>       drm_connector_cleanup(&hdmi->connector);
>>       drm_encoder_cleanup(&hdmi->encoder);
>> +     i2c_del_adapter(hdmi->i2c);
>>  }
>>
>>  static const struct component_ops sun4i_hdmi_ops = {
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c
>> new file mode 100644
>> index 000000000000..45cfd6bf483c
>> --- /dev/null
>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c
>> @@ -0,0 +1,242 @@
>> +/*
>> + * Copyright (C) 2016 Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx>
>> + * Copyright (C) 2017 Jonathan Liu <net147@xxxxxxxxx>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 of
>> + * the License, or (at your option) any later version.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/i2c.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/ktime.h>
>> +
>> +#include "sun4i_hdmi.h"
>> +
>> +#define SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK ( \
>> +     SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION | \
>> +     SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOW | \
>> +     SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW | \
>> +     SUN4I_HDMI_DDC_INT_STATUS_ARBITRATION_ERROR | \
>> +     SUN4I_HDMI_DDC_INT_STATUS_ACK_ERROR | \
>> +     SUN4I_HDMI_DDC_INT_STATUS_BUS_ERROR \
>> +)
>> +
>> +static int wait_fifo_flag_unset(struct sun4i_hdmi *hdmi, int len, u32 flag)
>> +{
>> +     /* 1 byte takes 9 clock cycles (8 bits + 1 ack) */
>> +     unsigned long byte_time = DIV_ROUND_UP(USEC_PER_SEC,
>> +                                            clk_get_rate(hdmi->ddc_clk)) * 9;
>> +     u32 reg;
>> +     ktime_t wait_timeout = ktime_add_us(ktime_get(), 100000);
>> +
>> +     for (;;) {
>> +             /* Check for errors */
>> +             reg = readl(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG);
>> +             if (reg & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK)
>> +                     return -EIO;
>> +
>> +             /* Check if requested FIFO flag is unset */
>> +             reg = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG);
>> +             if (!(reg & flag))
>> +                     break;
>> +
>> +             /* Timeout */
>> +             if (ktime_compare(ktime_get(), wait_timeout) > 0)
>> +                     return -ETIMEDOUT;
>> +
>> +             /* Wait for bytes to transfer (limited by FIFO size) */
>> +             usleep_range(byte_time,
>> +                          min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time);
>> +     }
>> +
>> +     /* Return FIFO level */
>> +     return (int)(reg & SUN4I_HDMI_DDC_FIFO_STATUS_LEVEL_MASK);
>> +}
>
> I still think you should be using readl_poll_timeout here, and then
> check for errors. This is both easier to understand and you avoid an
> unneeded duplication.
>

I will rework to use readl_poll_timeout.

>> +
>> +static int wait_fifo_read_ready(struct sun4i_hdmi *hdmi, int len)
>> +{
>> +     int level = wait_fifo_flag_unset(hdmi, len,
>> +                                      SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY);
>> +
>> +     /* Return number of bytes that can be read from FIFO */
>> +     return min(len, level);
>> +}
>
> The function's name is still inconsistent with what it does, and
> returns.
>

Will no longer be needed after reworking to use readl_poll_timeout.

>> +
>> +static int wait_fifo_write_ready(struct sun4i_hdmi *hdmi, int len)
>> +{
>> +     int level = wait_fifo_flag_unset(hdmi, len,
>> +                                      SUN4I_HDMI_DDC_FIFO_STATUS_FULL);
>> +
>> +     /* Return number of bytes that can be written to FIFO */
>> +     return min(len, SUN4I_HDMI_DDC_FIFO_SIZE - level);
>> +}
>
> Same thing here.
>

Will no longer be needed after reworking to use readl_poll_timeout.

>> +static int xfer_msg(struct sun4i_hdmi *hdmi, struct i2c_msg *msg)
>> +{
>> +     int i, len;
>> +     u32 reg;
>> +
>> +     /* Clear errors */
>> +     reg = readl(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG);
>> +     reg &= ~SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK;
>> +     writel(reg, hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG);
>> +
>> +     /* Set FIFO direction */
>> +     reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
>> +     reg &= ~SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK;
>> +     reg |= (msg->flags & I2C_M_RD) ?
>> +            SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ :
>> +            SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE;
>> +     writel(reg, hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
>> +
>> +     /* Set I2C address */
>> +     writel(SUN4I_HDMI_DDC_ADDR_SLAVE(msg->addr),
>> +            hdmi->base + SUN4I_HDMI_DDC_ADDR_REG);
>> +
>> +     /* Clear FIFO */
>> +     reg = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG);
>> +     writel(reg | SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR,
>> +            hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG);
>> +     if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG,
>> +                            reg,
>> +                            !(reg & SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR),
>> +                            100, 100000))
>> +             return -EIO;
>> +
>> +     /* Set transfer length */
>> +     writel(msg->len, hdmi->base + SUN4I_HDMI_DDC_BYTE_COUNT_REG);
>> +
>> +     /* Set command */
>> +     writel(msg->flags & I2C_M_RD ?
>> +            SUN4I_HDMI_DDC_CMD_IMPLICIT_READ :
>> +            SUN4I_HDMI_DDC_CMD_IMPLICIT_WRITE,
>> +            hdmi->base + SUN4I_HDMI_DDC_CMD_REG);
>> +
>> +     /* Start command */
>> +     reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
>> +     writel(reg | SUN4I_HDMI_DDC_CTRL_START_CMD,
>> +            hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
>> +
>> +     /* Transfer bytes */
>> +     if (msg->flags & I2C_M_RD) {
>> +             for (i = 0; i < msg->len; i += len) {
>> +                     len = wait_fifo_read_ready(hdmi, msg->len - i);
>> +                     if (len <= 0)
>> +                             return len;
>> +
>> +                     readsb(hdmi->base + SUN4I_HDMI_DDC_FIFO_DATA_REG,
>> +                            msg->buf + i, len);
>> +             }
>> +     } else {
>> +             for (i = 0; i < msg->len; i += len) {
>> +                     len = wait_fifo_write_ready(hdmi, msg->len - i);
>> +                     if (len <= 0)
>> +                             return len;
>> +
>> +                     writesb(hdmi->base + SUN4I_HDMI_DDC_FIFO_DATA_REG,
>> +                             msg->buf + i, len);
>> +             }
>> +     }
>> +
>> +     /* Wait for command to finish */
>> +     if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG,
>> +                            reg,
>> +                            !(reg & SUN4I_HDMI_DDC_CTRL_START_CMD),
>> +                            100, 100000))
>> +             return -EIO;
>> +
>> +     /* Check for errors */
>> +     reg = readl(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG);
>> +     if ((reg & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK) ||
>> +         !(reg & SUN4I_HDMI_DDC_INT_STATUS_TRANSFER_COMPLETE)) {
>> +             return -EIO;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int sun4i_hdmi_i2c_xfer(struct i2c_adapter *adap,
>> +                            struct i2c_msg *msgs, int num)
>> +{
>> +     struct sun4i_hdmi *hdmi = i2c_get_adapdata(adap);
>> +     u32 reg;
>> +     int err, i, ret = num;
>> +
>> +     for (i = 0; i < num; i++) {
>> +             if (!msgs[i].len)
>> +                     return -EINVAL;
>> +             if (msgs[i].len > SUN4I_HDMI_DDC_BYTE_COUNT_MAX)
>> +                     return -EINVAL;
>> +     }
>> +
>> +     /* Reset I2C controller */
>> +     writel(SUN4I_HDMI_DDC_CTRL_ENABLE | SUN4I_HDMI_DDC_CTRL_RESET,
>> +            hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
>> +     if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG, reg,
>> +                            !(reg & SUN4I_HDMI_DDC_CTRL_RESET),
>> +                            100, 2000))
>> +             return -EIO;
>> +
>> +     writel(SUN4I_HDMI_DDC_LINE_CTRL_SDA_ENABLE |
>> +            SUN4I_HDMI_DDC_LINE_CTRL_SCL_ENABLE,
>> +            hdmi->base + SUN4I_HDMI_DDC_LINE_CTRL_REG);
>> +
>> +     clk_prepare_enable(hdmi->ddc_clk);
>> +     clk_set_rate(hdmi->ddc_clk, 100000);
>> +
>> +     for (i = 0; i < num; i++) {
>> +             err = xfer_msg(hdmi, &msgs[i]);
>> +             if (err) {
>> +                     ret = err;
>> +                     break;
>> +             }
>> +     }
>> +
>> +     clk_disable_unprepare(hdmi->ddc_clk);
>> +     return ret;
>> +}
>> +
>> +static u32 sun4i_hdmi_i2c_func(struct i2c_adapter *adap)
>> +{
>> +     return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
>> +}
>> +
>> +static const struct i2c_algorithm sun4i_hdmi_i2c_algorithm = {
>> +     .master_xfer    = sun4i_hdmi_i2c_xfer,
>> +     .functionality  = sun4i_hdmi_i2c_func,
>> +};
>> +
>> +static const struct i2c_adapter sun4i_hdmi_i2c_adapter = {
>> +     .owner  = THIS_MODULE,
>> +     .class  = I2C_CLASS_DDC,
>> +     .algo   = &sun4i_hdmi_i2c_algorithm,
>> +     .name   = "sun4i_hdmi_i2c adapter",
>> +};
>> +
>> +int sun4i_hdmi_i2c_create(struct device *dev, struct sun4i_hdmi *hdmi)
>> +{
>> +     struct i2c_adapter *adap;
>> +     int ret = 0;
>> +
>> +     ret = sun4i_ddc_create(hdmi, hdmi->tmds_clk);
>> +     if (ret)
>> +             return ret;
>> +
>> +     adap = devm_kmemdup(dev, &sun4i_hdmi_i2c_adapter,
>> +                         sizeof(sun4i_hdmi_i2c_adapter), GFP_KERNEL);
>
> It would be more consistent with what the kernel usually does to just
> kmalloc that structure, and fill it.
>

Ok.

> Thanks!
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Thanks for the reviews.

Regards,
Jonathan