[PATCH v4 3/3] drm/bridge: add Silicon Image SiI8620 driver

[Laurent Pinchart]

Hi Andrzej,

Thank you for the patch.

On Friday 07 Oct 2016 09:02:42 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>
> ---
> v4:
>   - updated mhl.h location
> v3:
>   - modified driver description,
>   - removed dummy bridge callbacks,
>   - removed locking from driver remove function.
> ---
>  drivers/gpu/drm/bridge/Kconfig       |    7 +
>  drivers/gpu/drm/bridge/Makefile      |    1 +
>  drivers/gpu/drm/bridge/sil-sii8620.c | 1557 +++++++++++++++++++++++++++++++
>  drivers/gpu/drm/bridge/sil-sii8620.h | 1517 ++++++++++++++++++++++++++++++
>  4 files changed, 3082 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

Shouldn't you depend on I2C ?

> +	help
> +	  Silicon Image SII8620 HDMI/MHL bridge chip driver.
> +
>  config DRM_SII902X
>  	tristate "Silicon Image sii902x RGB/HDMI bridge"
>  	depends on OF

[snip]

> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c
> b/drivers/gpu/drm/bridge/sil-sii8620.c new file mode 100644
> index 0000000..7f053a5
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c

[snip]

> +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);

As len is 2 at most this seems overkill...

> +		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 }; \

You can make this const. Furthermore, given that there's quite a few calls to 
this macro with lots of compile-time static arguments, a static const version 
of the macro would be useful.

> +	__sii8620_write_seq(ctx, d, ARRAY_SIZE(d)); \
> +})

[snip]

> +static const struct of_device_id sii8620_dt_match[] = {
> +	{ .compatible = "sil,sii8620" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, sii8620_dt_match);
> +
> +static const struct i2c_device_id sii8620_id[] = {
> +	{ "SII8620", 0 },

Don't I2C IDs usually use lower case in the kernel ?

> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, sii8620_id);

-- 
Regards,

Laurent Pinchart