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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Maxime,

On 21 June 2017 at 18:51, Maxime Ripard
<maxime.ripard@xxxxxxxxxxxxxxxxxx> 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 <net147@xxxxxxxxx>
>> ---
>> 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_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,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_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_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_REG        0x51c
>>
>>  #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)
>> @@ -123,6 +140,7 @@
>>  #define SUN4I_HDMI_DDC_LINE_CTRL_SCL_ENABLE  BIT(8)
>>
>>  #define SUN4I_HDMI_DDC_FIFO_SIZE     16
>> +#define SUN4I_HDMI_DDC_MAX_TRANSFER_SIZE   1023
>
> Where is that limit mentionned ?
It is the width of the DDC_Access_Data_Byte_Number field in
DDC_Byte_Counter register.

How about moving it and renaming it like this?
#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)

Grouping it and having it similar name would be clearer.

>
>>
>>  enum sun4i_hdmi_pkt_type {
>>       SUN4I_HDMI_PKT_AVI = 2,
>> @@ -146,6 +164,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 +173,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 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..57a9a7a13e02 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(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..e82fa8a55165
>> --- /dev/null
>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c
>> @@ -0,0 +1,220 @@
>> +/*
>> + * Copyright (C) 2017 Jonathan Liu
>> + *
>> + * 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_UNDERFLOW | \
>> +     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, 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(), 100000);
>> +     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). 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. 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, 100000))
>> +             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.

>
>> +
>> +                     *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 one byte at a time. This is my
first kernel driver so I still have a lot to learn. In the future it
can be improved by using DMA and interrupts.

>> +             }
>> +     } else {
>> +             for (i = 0; i < msg->len; i++) {
>> +                     if (wait_fifo_write_ready(hdmi))
>> +                             return -EIO;
>> +
>> +                     writeb(*msg->buf++, hdmi->base +
>> +                            SUN4I_HDMI_DDC_FIFO_DATA_REG);
>> +             }
>> +     }
>> +
>> +     if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG,
>> +                            reg,
>> +                            !(reg & SUN4I_HDMI_DDC_CTRL_START_CMD),
>> +                            100, 100000))
>> +             return -EIO;
>> +
>> +     reg = readl(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG);
>> +
>> +     /* Check for errors */
>> +     if ((reg & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK) ||
>> +         !(reg & SUN4I_HDMI_DDC_INT_STATUS_TRANSFER_COMPLETE)) {
>> +             writel(reg, hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG);
>> +             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_MAX_TRANSFER_SIZE)
>> +                     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 struct i2c_adapter sun4i_hdmi_i2c_adapter = {
>> +     .owner  = THIS_MODULE,
>> +     .class  = I2C_CLASS_DDC,
>> +     .algo   = &sun4i_hdmi_i2c_algorithm,
>> +     .name   = "sun4i_hdmi_i2c adapter",
>> +};
>
> const?

i2c_add_adapter and i2c_del_adapter take "struct i2c_adapter *adapter"
argument so I can't make it const.

>
> (plus the changes mentionned in the v2 I just replied to, if they
> still apply).
Ok.

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

Regards,
Jonathan
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux