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
