Hi Laurent,
Thanks for review.
On 07.10.2016 16:58, Laurent Pinchart wrote:
> 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@xxxxxxxxxxx>
>> ---
>> 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 ?
It is under DRM_BRIDGE, which depends on DRM, which
selects 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...
Hmm, it is used multiple times with bigger buffers, see
sii8620_set_dev_cap, or edid related code.
>
>> + 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.
You are right.
>
>> + __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 ?
OK.
Regards
Andrzej
>
>> + { },
>> +};
>> +
>> +MODULE_DEVICE_TABLE(i2c, sii8620_id);
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
