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

Jonathan Liu net147 at gmail.com
Sun Jun 11 22:47:30 PDT 2017


Hi Chen-Yu,

On 12 June 2017 at 13:28, Chen-Yu Tsai <wens at csie.org> wrote:
> On Mon, Jun 12, 2017 at 10:12 AM, Jonathan Liu <net147 at gmail.com> wrote:
>> The drm_get_edid function should be used instead of drm_do_get_edid by
>> exposing the DDC bus as an I2C adapter. Implement this for A10s.
>
> Nice!

It is my first Linux kernel driver so there may be other things I have
overlooked. I am not sure about the behaviour of I2C writes larger
than 16 bytes but the EEPROMs I have tested only have a write buffer
of 8 bytes so writes are limited to a maximum of 8 bytes at a time
anyway.

>
>>
>> Signed-off-by: Jonathan Liu <net147 at gmail.com>
>> ---
>>  drivers/gpu/drm/sun4i/Makefile         |   1 +
>>  drivers/gpu/drm/sun4i/sun4i_hdmi.h     |  11 ++-
>>  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c |  96 +++----------------
>>  drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 166 +++++++++++++++++++++++++++++++++
>>  4 files changed, 190 insertions(+), 84 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 2589bc92be59..8e7a70f67f04 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> @@ -100,6 +100,7 @@
>>  #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_READ      (0 << 8)
>> +#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE     (1 << 8)
>>  #define SUN4I_HDMI_DDC_CTRL_RESET              BIT(0)
>>
>>  #define SUN4I_HDMI_DDC_ADDR_REG                0x504
>> @@ -108,6 +109,10 @@
>>  #define SUN4I_HDMI_DDC_ADDR_OFFSET(off)                (((off) & 0xff) << 8)
>>  #define SUN4I_HDMI_DDC_ADDR_SLAVE(addr)                ((addr) & 0xff)
>>
>> +#define SUN4I_HDMI_DDC_INTERRUPT_STATUS_REG    0x50c
>> +#define SUN4I_HDMI_DDC_INTERRUPT_STATUS_FIFO_REQUEST   BIT(4)
>> +#define SUN4I_HDMI_DDC_INTERRUPT_STATUS_TRANSFER_COMPLETE      BIT(0)
>> +
>>  #define SUN4I_HDMI_DDC_FIFO_CTRL_REG   0x510
>>  #define SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR         BIT(31)
>>
>> @@ -115,7 +120,8 @@
>>  #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_WRITE      3
>> +#define SUN4I_HDMI_DDC_CMD_IMPLICIT_READ       5
>>
>>  #define SUN4I_HDMI_DDC_CLK_REG         0x528
>>  #define SUN4I_HDMI_DDC_CLK_M(m)                        (((m) & 0x7) << 3)
>> @@ -181,6 +187,8 @@ struct sun4i_hdmi {
>>         struct clk              *ddc_clk;
>>         struct clk              *tmds_clk;
>>
>> +       struct i2c_adapter      *i2c;
>> +
>>         struct sun4i_drv        *drv;
>>
>>         bool                    hdmi_monitor;
>> @@ -192,5 +200,6 @@ int sun4i_ddc_create(struct sun4i_hdmi *hdmi, struct clk *clk);
>>  int sun6i_ddc_create(struct sun4i_hdmi *hdmi, struct clk *clk);
>>  int sun4i_tmds_create(struct sun4i_hdmi *hdmi);
>>  int sun6i_tmds_create(struct sun4i_hdmi *hdmi);
>> +int sun4i_hdmi_i2c_create(struct sun4i_hdmi *hdmi);
>
> However you seem to have based it on the wrong branch/patches.
> You based them on my A31 HDMI patches, instead of what Maxime has
> in his sunxi-drm/for-next branch.
>
> I could add this to my A31 patches, though it probably won't make
> 4.13. Alternatively you could rebase them on top of Maxime's branch.

Yes, I forgot about that. I will rebase against sunxi-drm/for-next for V2.

>
>>
>>  #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 e9abf93eb41c..6c9b11c4cf68 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> @@ -186,93 +186,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;
>>
>> @@ -284,8 +204,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;
>>  }
>>
>> @@ -424,6 +342,7 @@ struct sun4i_hdmi_variant {
>>         const struct drm_connector_helper_funcs *connector_helpers;
>>         int (*ddc_create)(struct sun4i_hdmi *hdmi, struct clk *clk);
>>         int (*tmds_create)(struct sun4i_hdmi *hdmi);
>> +       int (*hdmi_i2c_create)(struct sun4i_hdmi *hdmi);
>>         bool has_ddc_parent_clk;
>>         bool has_reset_control;
>>
>> @@ -439,6 +358,7 @@ static const struct sun4i_hdmi_variant sun5i_variant = {
>>         .connector_helpers      = &sun4i_hdmi_connector_helper_funcs,
>>         .ddc_create             = sun4i_ddc_create,
>>         .tmds_create            = sun4i_tmds_create,
>> +       .hdmi_i2c_create        = sun4i_hdmi_i2c_create,
>>         .has_ddc_parent_clk     = false,
>>         .has_reset_control      = false,
>>         .pad_ctrl0_init_val     = SUN4I_HDMI_PAD_CTRL0_TXEN |
>> @@ -622,6 +542,14 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
>>                 goto err_disable_mod_clk;
>>         }
>>
>> +       if (hdmi->variant->hdmi_i2c_create) {
>
> I don't think this would be optional.

This will be addressed in V2 when the patch is rebased.

>
>> +               ret = hdmi->variant->hdmi_i2c_create(hdmi);
>> +               if (ret) {
>> +                       dev_err(dev, "Couldn't create the HDMI I2C adapter\n");
>> +                       goto err_disable_mod_clk;
>> +               }
>> +       }
>> +
>
> You should also fix up the error path so that the I2C adapter
> gets remove if any of the subsequent calls fail.
>
>>         drm_encoder_helper_add(&hdmi->encoder,
>>                                &sun4i_hdmi_helper_funcs);
>>         ret = drm_encoder_init(drm,
>> @@ -676,6 +604,8 @@ static void sun4i_hdmi_unbind(struct device *dev, struct device *master,
>>
>>         drm_connector_cleanup(&hdmi->connector);
>>         drm_encoder_cleanup(&hdmi->encoder);
>> +       if (hdmi->i2c)
>> +               i2c_del_adapter(hdmi->i2c);
>
> So neither should this be optional.

Ditto. This will be addressed in V2 when the patch is rebased.

>
>>         clk_disable_unprepare(hdmi->mod_clk);
>>         clk_disable_unprepare(hdmi->bus_clk);
>>  }
>> 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..c33fa13cc668
>> --- /dev/null
>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c
>> @@ -0,0 +1,166 @@
>> +/*
>> + * Copyright (C) 2017 Jonathan Liu
>> + *
>> + * Jonathan Liu <net147 at gmail.com>
>> + *
>> + * 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/init.h>
>> +#include <linux/module.h>
>> +#include <linux/errno.h>
>> +#include <linux/clk.h>
>> +#include <linux/i2c.h>
>> +#include <linux/iopoll.h>
>> +
>> +#include "sun4i_hdmi.h"
>> +
>> +static int xfer_msg_chunk(struct sun4i_hdmi *hdmi, struct i2c_msg *msg)
>> +{
>> +       u32 count = min_t(u32, msg->len, SUN4I_HDMI_DDC_FIFO_SIZE);
>> +       u32 reg;
>> +       int i;
>> +
>> +       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(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 < count; i++) {
>> +                       writeb(*msg->buf++, hdmi->base
>> +                              + SUN4I_HDMI_DDC_FIFO_DATA_REG);
>> +                       --msg->len;
>> +               }
>> +       }
>> +
>> +       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_INTERRUPT_STATUS_REG);
>> +       reg &= ~SUN4I_HDMI_DDC_INTERRUPT_STATUS_FIFO_REQUEST;
>> +
>> +       if (reg != SUN4I_HDMI_DDC_INTERRUPT_STATUS_TRANSFER_COMPLETE)
>> +               return -EIO;
>> +
>> +       if (msg->flags & I2C_M_RD) {
>> +               for (i = 0; i < count; i++) {
>> +                       *msg->buf++ = readb(hdmi->base
>> +                                           + SUN4I_HDMI_DDC_FIFO_DATA_REG);
>> +                       --msg->len;
>> +               }
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int xfer_msg(struct sun4i_hdmi *hdmi, struct i2c_msg *msg)
>> +{
>> +       u32 reg;
>> +       int ret;
>> +
>> +       if (!msg->len)
>> +               return -EIO;
>> +
>> +       while (msg->len) {
>> +               reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
>> +               reg &= ~SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK;
>> +
>> +               writel(reg | (msg->flags & I2C_M_RD
>> +                             ? SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ
>> +                             : SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE),
>> +                      hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
>> +
>> +               writel(SUN4I_HDMI_DDC_ADDR_SLAVE(msg->addr),
>> +                      hdmi->base + SUN4I_HDMI_DDC_ADDR_REG);
>> +
>> +               ret = xfer_msg_chunk(hdmi, msg);
>> +               if (ret)
>> +                       return ret;
>> +       }
>> +
>> +       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, ret = num;
>> +       int i;
>> +
>> +       /* 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",
>> +};
>> +
>> +int sun4i_hdmi_i2c_create(struct sun4i_hdmi *hdmi)
>> +{
>> +       int ret = 0;
>> +
>> +       i2c_set_adapdata(&sun4i_hdmi_i2c_adapter, hdmi);
>> +
>> +       ret = i2c_add_adapter(&sun4i_hdmi_i2c_adapter);
>> +       if (ret)
>> +               return ret;
>> +
>> +       hdmi->i2c = &sun4i_hdmi_i2c_adapter;
>> +
>> +       return ret;
>> +}
>> --
>> 2.12.2
>>
>
> The rest looks good to me.
>
> Regards
> ChenYu

Will follow up with V2 shortly.

Regards,
Jonathan



More information about the linux-arm-kernel mailing list