[PATCH 2nd RESEND v2 3/3] drm/bridge: add Silicon Image SiI8620 driver

Archit Taneja architt at codeaurora.org
Mon Sep 26 11:35:15 UTC 2016



On 09/26/2016 05:01 PM, Archit Taneja wrote:
>
>
> On 09/26/2016 03:06 PM, Andrzej Hajda wrote:
>> Hi,
>>
>> Thanks for review.
>>
>>
>> On 25.09.2016 19:01, Archit Taneja wrote:
>>> Hi,
>>>
>>> On 9/14/2016 2:03 PM, Andrzej Hajda wrote:
>>>> SiI8620 transmitter converts eTMDS/HDMI signal to MHL 3.0.
>>>> It is controlled via I2C bus. Its interaction with other
>>>> devices in video pipeline is performed mainly on HW level.
>>>> The only interaction it does on device driver level is
>>>> filtering-out unsupported video modes, it exposes drm_bridge
>>>> interface to perform this operation.
>>>>
>>>> Signed-off-by: Andrzej Hajda <a.hajda at samsung.com>
>>>> ---
>>>>   drivers/gpu/drm/bridge/Kconfig       |    7 +
>>>>   drivers/gpu/drm/bridge/Makefile      |    1 +
>>>>   drivers/gpu/drm/bridge/sil-sii8620.c | 1568
>>>> ++++++++++++++++++++++++++++++++++
>>>>   drivers/gpu/drm/bridge/sil-sii8620.h | 1517
>>>> ++++++++++++++++++++++++++++++++
>>>>   4 files changed, 3093 insertions(+)
>>>>   create mode 100644 drivers/gpu/drm/bridge/sil-sii8620.c
>>>>   create mode 100644 drivers/gpu/drm/bridge/sil-sii8620.h
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/Kconfig
>>>> b/drivers/gpu/drm/bridge/Kconfig
>>>> index b590e67..e7fb179 100644
>>>> --- a/drivers/gpu/drm/bridge/Kconfig
>>>> +++ b/drivers/gpu/drm/bridge/Kconfig
>>>> @@ -50,6 +50,13 @@ config DRM_PARADE_PS8622
>>>>       ---help---
>>>>         Parade eDP-LVDS bridge chip driver.
>>>>
>>>> +config DRM_SIL_SII8620
>>>> +    tristate "Silicon Image SII8620 HDMI/MHL bridge"
>>>> +    depends on OF
>>>> +    select DRM_KMS_HELPER
>>>> +    help
>>>> +      Silicon Image SII8620 HDMI/MHL bridge chip driver.
>>>> +
>>>>   config DRM_SII902X
>>>>       tristate "Silicon Image sii902x RGB/HDMI bridge"
>>>>       depends on OF
>>>> diff --git a/drivers/gpu/drm/bridge/Makefile
>>>> b/drivers/gpu/drm/bridge/Makefile
>>>> index efdb07e..304cf9d 100644
>>>> --- a/drivers/gpu/drm/bridge/Makefile
>>>> +++ b/drivers/gpu/drm/bridge/Makefile
>>>> @@ -5,6 +5,7 @@ obj-$(CONFIG_DRM_DW_HDMI) += dw-hdmi.o
>>>>   obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw-hdmi-ahb-audio.o
>>>>   obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
>>>>   obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
>>>> +obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
>>>>   obj-$(CONFIG_DRM_SII902X) += sii902x.o
>>>>   obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
>>>>   obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>>>> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c
>>>> b/drivers/gpu/drm/bridge/sil-sii8620.c
>>>> new file mode 100644
>>>> index 0000000..3b273c7
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
>>>> @@ -0,0 +1,1568 @@
>>>> +/*
>>>> + * Driver for Silicon Image SiI8620 Mobile HD Transmitter
>>> Would be nice to mention "eTMDS/HDMI signal to MHL 3.0." here, so that
>>> people know what it's used for without going through the rest of the
>>> driver.
>>
>> OK
>>
>>>
>>>> + *
>>>> + * Copyright (C) 2015, Samsung Electronics Co., Ltd.
>>>> + * Andrzej Hajda <a.hajda at samsung.com>
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or
>>>> modify
>>>> + * it under the terms of the GNU General Public License version 2 as
>>>> + * published by the Free Software Foundation.
>>>> + */
>>>> +
>>>> +#include <drm/drm_crtc.h>
>>>> +#include <drm/drm_edid.h>
>>>> +
>>>> +#include <linux/clk.h>
>>>> +#include <linux/delay.h>
>>>> +#include <linux/gpio/consumer.h>
>>>> +#include <linux/i2c.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/irq.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/list.h>
>>>> +#include <linux/mhl.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/mutex.h>
>>>> +#include <linux/regulator/consumer.h>
>>>> +#include <linux/slab.h>
>>>> +
>>>> +#include "sil-sii8620.h"
>>>> +
>>>> +#define VAL_RX_HDMI_CTRL2_DEFVAL    VAL_RX_HDMI_CTRL2_IDLE_CNT(3)
>>>> +
>>>> +enum sii8620_mode {
>>>> +    CM_DISCONNECTED,
>>>> +    CM_DISCOVERY,
>>>> +    CM_MHL1,
>>>> +    CM_MHL3,
>>>> +    CM_ECBUS_S
>>>> +};
>>>> +
>>>> +enum sii8620_sink_type {
>>>> +    SINK_NONE,
>>>> +    SINK_HDMI,
>>>> +    SINK_DVI
>>>> +};
>>>> +
>>>> +enum sii8620_mt_state {
>>>> +    MT_STATE_READY,
>>>> +    MT_STATE_BUSY,
>>>> +    MT_STATE_DONE
>>>> +};
>>>> +
>>>> +struct sii8620 {
>>>> +    struct drm_bridge bridge;
>>>> +    struct device *dev;
>>>> +    struct clk *clk_xtal;
>>>> +    struct gpio_desc *gpio_reset;
>>>> +    struct gpio_desc *gpio_int;
>>>> +    struct regulator_bulk_data supplies[2];
>>>> +    struct mutex lock; /* context lock, protects fields below */
>>>> +    int error;
>>>> +    enum sii8620_mode mode;
>>>> +    enum sii8620_sink_type sink_type;
>>>> +    u8 cbus_status;
>>>> +    u8 stat[MHL_DST_SIZE];
>>>> +    u8 xstat[MHL_XDS_SIZE];
>>>> +    u8 devcap[MHL_DCAP_SIZE];
>>>> +    u8 xdevcap[MHL_XDC_SIZE];
>>>> +    u8 avif[19];
>>>> +    struct edid *edid;
>>>> +    unsigned int gen2_write_burst:1;
>>>> +    enum sii8620_mt_state mt_state;
>>>> +    struct list_head mt_queue;
>>>> +};
>>>> +
>>>> +struct sii8620_mt_msg;
>>>> +
>>>> +typedef void (*sii8620_mt_msg_cb)(struct sii8620 *ctx,
>>>> +                  struct sii8620_mt_msg *msg);
>>>> +
>>>> +struct sii8620_mt_msg {
>>>> +    struct list_head node;
>>>> +    u8 reg[4];
>>>> +    u8 ret;
>>>> +    sii8620_mt_msg_cb send;
>>>> +    sii8620_mt_msg_cb recv;
>>>> +};
>>>> +
>>>> +static const u8 sii8620_i2c_page[] = {
>>>> +    0x39, /* Main System */
>>>> +    0x3d, /* TDM and HSIC */
>>>> +    0x49, /* TMDS Receiver, MHL EDID */
>>>> +    0x4d, /* eMSC, HDCP, HSIC */
>>>> +    0x5d, /* MHL Spec */
>>>> +    0x64, /* MHL CBUS */
>>>> +    0x59, /* Hardware TPI (Transmitter Programming Interface) */
>>>> +    0x61, /* eCBUS-S, eCBUS-D */
>>>> +};
>>>> +
>>>> +static void sii8620_fetch_edid(struct sii8620 *ctx);
>>>> +static void sii8620_set_upstream_edid(struct sii8620 *ctx);
>>>> +static void sii8620_enable_hpd(struct sii8620 *ctx);
>>>> +static void sii8620_mhl_disconnected(struct sii8620 *ctx);
>>>> +
>>>> +static int sii8620_clear_error(struct sii8620 *ctx)
>>>> +{
>>>> +    int ret = ctx->error;
>>>> +
>>>> +    ctx->error = 0;
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static void sii8620_read_buf(struct sii8620 *ctx, u16 addr, u8
>>>> *buf, int len)
>>>> +{
>>>> +    struct device *dev = ctx->dev;
>>>> +    struct i2c_client *client = to_i2c_client(dev);
>>>> +    u8 data = addr;
>>>> +    struct i2c_msg msg[] = {
>>>> +        {
>>>> +            .addr = sii8620_i2c_page[addr >> 8],
>>>> +            .flags = client->flags,
>>>> +            .len = 1,
>>>> +            .buf = &data
>>>> +        },
>>>> +        {
>>>> +            .addr = sii8620_i2c_page[addr >> 8],
>>>> +            .flags = client->flags | I2C_M_RD,
>>>> +            .len = len,
>>>> +            .buf = buf
>>>> +        },
>>>> +    };
>>>> +    int ret;
>>>> +
>>>> +    if (ctx->error)
>>>> +        return;
>>>> +
>>>> +    ret = i2c_transfer(client->adapter, msg, 2);
>>>> +    dev_dbg(dev, "read at %04x: %*ph, %d\n", addr, len, buf, ret);
>>>> +
>>>> +    if (ret != 2) {
>>>> +        dev_err(dev, "Read at %#06x of %d bytes failed with code
>>>> %d.\n",
>>>> +            addr, len, ret);
>>>> +        ctx->error = ret < 0 ? ret : -EIO;
>>>> +    }
>>>> +}
>>>> +
>>>> +static u8 sii8620_readb(struct sii8620 *ctx, u16 addr)
>>>> +{
>>>> +    u8 ret;
>>>> +
>>>> +    sii8620_read_buf(ctx, addr, &ret, 1);
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static void sii8620_write_buf(struct sii8620 *ctx, u16 addr, const
>>>> u8 *buf,
>>>> +                  int len)
>>>> +{
>>>> +    struct device *dev = ctx->dev;
>>>> +    struct i2c_client *client = to_i2c_client(dev);
>>>> +    u8 data[2];
>>>> +    struct i2c_msg msg = {
>>>> +        .addr = sii8620_i2c_page[addr >> 8],
>>>> +        .flags = client->flags,
>>>> +        .len = len + 1,
>>>> +    };
>>>> +    int ret;
>>>> +
>>>> +    if (ctx->error)
>>>> +        return;
>>>> +
>>>> +    if (len > 1) {
>>>> +        msg.buf = kmalloc(len + 1, GFP_KERNEL);
>>>> +        if (!msg.buf) {
>>>> +            ctx->error = -ENOMEM;
>>>> +            return;
>>>> +        }
>>>> +        memcpy(msg.buf + 1, buf, len);
>>>> +    } else {
>>>> +        msg.buf = data;
>>>> +        msg.buf[1] = *buf;
>>>> +    }
>>>> +
>>>> +    msg.buf[0] = addr;
>>>> +
>>>> +    ret = i2c_transfer(client->adapter, &msg, 1);
>>>> +    dev_dbg(dev, "write at %04x: %*ph, %d\n", addr, len, buf, ret);
>>>> +
>>>> +    if (ret != 1) {
>>>> +        dev_err(dev, "Write at %#06x of %*ph failed with code %d.\n",
>>>> +            addr, len, buf, ret);
>>>> +        ctx->error = ret ?: -EIO;
>>>> +    }
>>>> +
>>>> +    if (len > 1)
>>>> +        kfree(msg.buf);
>>>> +}
>>>> +
>>>> +#define sii8620_write(ctx, addr, arr...) \
>>>> +({\
>>>> +    u8 d[] = { arr }; \
>>>> +    sii8620_write_buf(ctx, addr, d, ARRAY_SIZE(d)); \
>>>> +})
>>>> +
>>>> +static void __sii8620_write_seq(struct sii8620 *ctx, u16 *seq, int
>>>> len)
>>>> +{
>>>> +    int i;
>>>> +
>>>> +    for (i = 0; i < len; i += 2)
>>>> +        sii8620_write(ctx, seq[i], seq[i + 1]);
>>>> +}
>>>> +
>>>> +#define sii8620_write_seq(ctx, seq...) \
>>>> +({\
>>>> +    u16 d[] = { seq }; \
>>>> +    __sii8620_write_seq(ctx, d, ARRAY_SIZE(d)); \
>>>> +})
>>>> +
>>> Is it possible to use regmap_bulk_read/write api here instead?
>>
>> sii8620 address space is divided into 8 pages (see sii8620_i2c_page
>> variable), every page is mapped to separate i2c slave address.
>> I/O access helpers defined above works with this address space,
>> it is important especially for sii8620_write_seq, which allow to set
>> registers located in different pages within one call.
>>
>> Theoretically it could be possible to register separate regmap for every
>> page and rewrite helpers above to use them, but the burden seems
>> to be similar or bigger and no real benefit.
>
> Okay, thanks for the clarification. I guess it doesn't make much
> sense to use regmap here.
>
>>>
>>>> +static void sii8620_setbits(struct sii8620 *ctx, u16 addr, u8 mask,
>>>> u8 val)
>>>> +{
>>>> +    val = (val & mask) | (sii8620_readb(ctx, addr) & ~mask);
>>>> +    sii8620_write(ctx, addr, val);
>>>> +}
>>>> +
>>
>> (...)
>>
>>>> +
>>>> +static inline struct sii8620 *bridge_to_sii8620(struct drm_bridge
>>>> *bridge)
>>>> +{
>>>> +    return container_of(bridge, struct sii8620, bridge);
>>>> +}
>>>> +
>>>> +void sii8620_bridge_dummy(struct drm_bridge *bridge)
>>>> +{
>>>> +
>>>> +}
>>>> +
>>> We can now get rid of the dummy bridge ops and not populate anything.
>>
>> Yeah, world has changed between resends, I will remove it in next
>> iteration.
>>
>>>
>>>> +bool sii8620_mode_fixup(struct drm_bridge *bridge,
>>>> +           const struct drm_display_mode *mode,
>>>> +           struct drm_display_mode *adjusted_mode)
>>>> +{
>>>> +    struct sii8620 *ctx = bridge_to_sii8620(bridge);
>>>> +    bool ret = false;
>>>> +    int max_clock = 74250;
>>>> +
>>>> +    mutex_lock(&ctx->lock);
>>>> +
>>>> +    if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>>>> +        goto out;
>>>> +
>>>> +    if (ctx->devcap[MHL_DCAP_VID_LINK_MODE] &
>>>> MHL_DCAP_VID_LINK_PPIXEL)
>>>> +        max_clock = 300000;
>>>> +
>>>> +    ret = mode->clock <= max_clock;
>>>> +
>>>> +out:
>>>> +    mutex_unlock(&ctx->lock);
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +
>>>> +static const struct drm_bridge_funcs sii8620_bridge_funcs = {
>>>> +    .pre_enable = sii8620_bridge_dummy,
>>>> +    .enable = sii8620_bridge_dummy,
>>>> +    .disable = sii8620_bridge_dummy,
>>>> +    .post_disable = sii8620_bridge_dummy,
>>>> +    .mode_fixup = sii8620_mode_fixup,
>>>> +};
>>>> +
>>>> +static int sii8620_probe(struct i2c_client *client,
>>>> +             const struct i2c_device_id *id)
>>>> +{
>>>> +    struct device *dev = &client->dev;
>>>> +    struct sii8620 *ctx;
>>>> +    int ret;
>>>> +
>>>> +    ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
>>>> +    if (!ctx)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    ctx->dev = dev;
>>>> +    mutex_init(&ctx->lock);
>>>> +    INIT_LIST_HEAD(&ctx->mt_queue);
>>>> +
>>>> +    ctx->clk_xtal = devm_clk_get(dev, "xtal");
>>>> +    if (IS_ERR(ctx->clk_xtal)) {
>>>> +        dev_err(dev, "failed to get xtal clock from DT\n");
>>>> +        return PTR_ERR(ctx->clk_xtal);
>>>> +    }
>>>> +
>>>> +    if (!client->irq) {
>>>> +        dev_err(dev, "no irq provided\n");
>>>> +        return -EINVAL;
>>>> +    }
>>>> +    irq_set_status_flags(client->irq, IRQ_NOAUTOEN);
>>>> +    ret = devm_request_threaded_irq(dev, client->irq, NULL,
>>>> +            sii8620_irq_thread, IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
>>>> +            "sii8620", ctx);
>>>> +
>>>> +    ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
>>>> +    if (IS_ERR(ctx->gpio_reset)) {
>>>> +        dev_err(dev, "failed to get reset gpio from DT\n");
>>>> +        return PTR_ERR(ctx->gpio_reset);
>>>> +    }
>>>> +
>>>> +    ctx->supplies[0].supply = "cvcc10";
>>>> +    ctx->supplies[1].supply = "iovcc18";
>>>> +    ret = devm_regulator_bulk_get(dev, 2, ctx->supplies);
>>>> +    if (ret)
>>>> +        return ret;
>>>> +
>>>> +    i2c_set_clientdata(client, ctx);
>>>> +
>>>> +    ctx->bridge.funcs = &sii8620_bridge_funcs;
>>>> +    ctx->bridge.of_node = dev->of_node;
>>>> +    drm_bridge_add(&ctx->bridge);
>>>> +
>>>> +    sii8620_cable_in(ctx);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int sii8620_remove(struct i2c_client *client)
>>>> +{
>>>> +    struct sii8620 *ctx = i2c_get_clientdata(client);
>>>> +
>>>> +    disable_irq(to_i2c_client(ctx->dev)->irq);
>>>> +    mutex_lock(&ctx->lock);
>>>> +    drm_bridge_remove(&ctx->bridge);
>>> Do we need to call drm_bridge_remove with the lock held too?
>>
>> No, in fact after disabling irq there will be no concurrent access
>> to the context so locking here can be removed.
>>
>> On the other side drm_bridge_remove does nothing except removing
>> from bridge list, so it is possible that bridge can be unbound when it is
>> attached to working encoder, in such situation encoder will stay with
>> pointer to non-existing bridge - in such situation everything can happen.
>> I suppose this bug affects most bridges and panels and even if it
>> could be
>> fixed in bridge driver I guess drm_core should be better place for it.
>
> Right, I think if the kms driver using the bridge takes a reference of
> the bridge module during drm_bridge_attach, it would prevent removing
> the module until the kms driver doesn't call a drm_bridge_detach(), but
> I'm not sure if that's the right approach.
>
> Another option is to componentize the bridge drivers too, which would
> ensure that bridge components are bound before the master is bound, and
> unbound only after the master is unbound.

Oops, I meant the other way round.

Archit

>
> Will work on an RFC for this and get some opinions.
>
> Thanks,
> Archit
>
>>
>> Regards
>> Andrzej
>>
>>
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project


More information about the dri-devel mailing list