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

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@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@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.

Will work on an RFC for this and get some opinions.

Thanks, Archit

Regards Andrzej