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
