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