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

2017-06-25 Thread Jonathan Liu 
Hi Maxime,

On 22 June 2017 at 07:26, Maxime Ripard
 wrote:
> On Wed, Jun 21, 2017 at 07:42:47PM +1000, Jonathan Liu wrote:
>> >> +static int wait_fifo_flag_unset(struct sun4i_hdmi *hdmi, 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;
>> >> + ktime_t wait_timeout = ktime_add_us(ktime_get(), 10);
>> >> + u32 reg;
>> >> +
>> >> + for (;;) {
>> >> + /* Check for errors */
>> >> + reg = readl(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG);
>> >> + if (reg & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK) {
>> >> + writel(reg, hdmi->base + 
>> >> SUN4I_HDMI_DDC_INT_STATUS_REG);
>> >> + return -EIO;
>> >> + }
>> >> +
>> >> + /* Check if requested FIFO flag is unset */
>> >> + reg = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG);
>> >> + if (!(reg & flag))
>> >> + return 0;
>> >> +
>> >> + /* Timeout */
>> >> + if (ktime_compare(ktime_get(), wait_timeout) > 0)
>> >> + return -EIO;
>> >> +
>> >> + /* Wait for 1-2 bytes to transfer */
>> >> + usleep_range(byte_time, 2 * byte_time);
>> >> + }
>> >> +
>> >> + return -EIO;
>> >> +}
>> >> +
>> >> +static int wait_fifo_read_ready(struct sun4i_hdmi *hdmi)
>> >> +{
>> >> + return wait_fifo_flag_unset(hdmi, SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY);
>> >> +}
>> >> +
>> >> +static int wait_fifo_write_ready(struct sun4i_hdmi *hdmi)
>> >> +{
>> >> + return wait_fifo_flag_unset(hdmi, SUN4I_HDMI_DDC_FIFO_STATUS_FULL);
>> >> +}
>> >
>> > Both of these can use readl_poll_timeout, and I'm not sure you need to
>> > be that aggressive in your timings.
>> >
>> I have set my timings to minimize communication delays - e.g. when
>> userspace is reading from I2C one byte at a time (like i2cdump from
>> i2c-tools).
>
> I wouldn't try to optimise that too much. There's already a lot of
> overhead, it's way inefficient, and it's not really a significant use
> case anyway.
>

Ok.

>> readl_poll_timeout can't be used to check 2 registers at
>> once and I couldn't get DDC_FIFO_Request_Interrupt_Status_Bit in
>> DDC_Int_Status register to behave properly.
>
> Can't you just use readl_poll_timeout, and then if it timed out check
> for errors?
>

I think it is more flexible this way as I can adjust the usleep_range min.

>> I also wanted to minimize the chance of FIFO underrun/overrun.
>>
>> >> +
>> >> +static int xfer_msg(struct sun4i_hdmi *hdmi, struct i2c_msg *msg)
>> >> +{
>> >> + u32 reg;
>> >> + int i;
>> >> +
>> >> + 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);
>> >> +
>> >> + 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);
>> >> +
>> >> + writel(SUN4I_HDMI_DDC_ADDR_SLAVE(msg->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);
>> >> +
>> >> + if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG,
>> >> +reg,
>> >> +!(reg & SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR),
>> >> +100, 10))
>> >> + return -EIO;
>> >> +
>> >> + writel(msg->len, hdmi->base + SUN4I_HDMI_DDC_BYTE_COUNT_REG);
>> >> + 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);
>> >> +
>> >> + 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 (msg->flags & I2C_M_RD) {
>> >> + for (i = 0; i < msg->len; i++) {
>> >> + if (wait_fifo_read_ready(hdmi))
>> >> + return -EIO;
>> >
>> > it seems weird that a function called read_ready would return 1 if
>> > there's an error.
>>
>> I will change this so wait_fifo_flag_unset returns false on error
>> and true on success. Then the wait_fifo_read_ready and
>> wait_fifo_write_ready conditions will be inverted.
>
> Or just invert the return value, whatever makes

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

2017-06-21 Thread Jonathan Liu 
Hi Maxime,

On 21 June 2017 at 18:51, Maxime Ripard
 wrote:
> On Thu, Jun 15, 2017 at 01:29:33AM +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 
>> ---
>> 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 |  21 
>>  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 101 ++-
>>  drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 220 
>> +
>>  4 files changed, 253 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..018af9cbb593 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_CMDBIT(30)
>>  #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASKBIT(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_RESETBIT(0)
>>
>> @@ -105,14 +106,30 @@
>>  #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_REG0x50c
>> +#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_UNDERFLOW  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)
>> +
>
> The indentation of those bits and the ones above are weird, please
> check them.
>
I will fix the inconsistent indentation. I only just realised the
register field bits are indented more than the registry addresses.

>>  #define SUN4I_HDMI_DDC_FIFO_DATA_REG 0x518
>>  #define SUN4I_HDMI_DDC_BYTE_COUNT_REG0x51c
>>
>>  #define SUN4I_HDMI_DDC_CMD_REG   0x520
>>  #define SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ6
>> +#define

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

2017-06-21 Thread Maxime Ripard 
On Wed, Jun 21, 2017 at 07:42:47PM +1000, Jonathan Liu wrote:
> >> +static int wait_fifo_flag_unset(struct sun4i_hdmi *hdmi, 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;
> >> + ktime_t wait_timeout = ktime_add_us(ktime_get(), 10);
> >> + u32 reg;
> >> +
> >> + for (;;) {
> >> + /* Check for errors */
> >> + reg = readl(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG);
> >> + if (reg & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK) {
> >> + writel(reg, hdmi->base + 
> >> SUN4I_HDMI_DDC_INT_STATUS_REG);
> >> + return -EIO;
> >> + }
> >> +
> >> + /* Check if requested FIFO flag is unset */
> >> + reg = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG);
> >> + if (!(reg & flag))
> >> + return 0;
> >> +
> >> + /* Timeout */
> >> + if (ktime_compare(ktime_get(), wait_timeout) > 0)
> >> + return -EIO;
> >> +
> >> + /* Wait for 1-2 bytes to transfer */
> >> + usleep_range(byte_time, 2 * byte_time);
> >> + }
> >> +
> >> + return -EIO;
> >> +}
> >> +
> >> +static int wait_fifo_read_ready(struct sun4i_hdmi *hdmi)
> >> +{
> >> + return wait_fifo_flag_unset(hdmi, SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY);
> >> +}
> >> +
> >> +static int wait_fifo_write_ready(struct sun4i_hdmi *hdmi)
> >> +{
> >> + return wait_fifo_flag_unset(hdmi, SUN4I_HDMI_DDC_FIFO_STATUS_FULL);
> >> +}
> >
> > Both of these can use readl_poll_timeout, and I'm not sure you need to
> > be that aggressive in your timings.
> >
> I have set my timings to minimize communication delays - e.g. when
> userspace is reading from I2C one byte at a time (like i2cdump from
> i2c-tools).

I wouldn't try to optimise that too much. There's already a lot of
overhead, it's way inefficient, and it's not really a significant use
case anyway.

> readl_poll_timeout can't be used to check 2 registers at
> once and I couldn't get DDC_FIFO_Request_Interrupt_Status_Bit in
> DDC_Int_Status register to behave properly.

Can't you just use readl_poll_timeout, and then if it timed out check
for errors?

> I also wanted to minimize the chance of FIFO underrun/overrun.
> 
> >> +
> >> +static int xfer_msg(struct sun4i_hdmi *hdmi, struct i2c_msg *msg)
> >> +{
> >> + u32 reg;
> >> + int i;
> >> +
> >> + 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);
> >> +
> >> + 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);
> >> +
> >> + writel(SUN4I_HDMI_DDC_ADDR_SLAVE(msg->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);
> >> +
> >> + if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG,
> >> +reg,
> >> +!(reg & SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR),
> >> +100, 10))
> >> + return -EIO;
> >> +
> >> + writel(msg->len, hdmi->base + SUN4I_HDMI_DDC_BYTE_COUNT_REG);
> >> + 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);
> >> +
> >> + 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 (msg->flags & I2C_M_RD) {
> >> + for (i = 0; i < msg->len; i++) {
> >> + if (wait_fifo_read_ready(hdmi))
> >> + return -EIO;
> >
> > it seems weird that a function called read_ready would return 1 if
> > there's an error.
>
> I will change this so wait_fifo_flag_unset returns false on error
> and true on success. Then the wait_fifo_read_ready and
> wait_fifo_write_ready conditions will be inverted.

Or just invert the return value, whatever makes sense

> >
> >> +
> >> + *msg->buf++ = readb(hdmi->base +
> >> + SUN4I_HDMI_DDC_FIFO_DATA_REG);
> >
> > Don't you have an hardware FIFO you can rely on intead of reading
> > everything byte per byte?
> >
> Hardware FIFO by nature is transferring

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

2017-06-21 Thread Maxime Ripard 
On Thu, Jun 15, 2017 at 01:29:33AM +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 
> ---
> 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 |  21 
>  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 101 ++-
>  drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 220 
> +
>  4 files changed, 253 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..018af9cbb593 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_CMDBIT(30)
>  #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASKBIT(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_RESETBIT(0)
>  
> @@ -105,14 +106,30 @@
>  #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_REG0x50c
> +#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_UNDERFLOW  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)
> +

The indentation of those bits and the ones above are weird, please
check them.

>  #define SUN4I_HDMI_DDC_FIFO_DATA_REG 0x518
>  #define SUN4I_HDMI_DDC_BYTE_COUNT_REG0x51c
>  
>  #define SUN4I_HDMI_DDC_CMD_REG   0x520
>  #define SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ6
> +#define SUN4I_HDMI_DDC_CMD_IMPLICIT_READ 5
> +#define SUN4I_HDMI_DDC_CMD_IMPLICIT_WRITE3
>  
>  #define SUN4I_HDMI_DDC_CLK_REG   0x528
>  #define SUN4I_HDMI_DDC_CLK_M(m)  (((m) & 0x7) << 3)
> @@ -123,6 +140,7 @@
>  #define SUN4I_HDMI_DDC_LINE_CTRL_SCL_ENABLE  BIT(8)
>  
>  #define SUN4I_HDMI_DDC_FIFO_SIZE 16
>