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

[Andrzej Hajda]

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 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 ?

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