Re: [PATCH] Driver for Toshiba TC358743 HDMI to CSI-2 bridge

[Hans Verkuil <hverkuil@xs4all.nl>]

Hi Mats,

Here is one more:

On 05/06/15 11:27, matrandg@cisco.com wrote:
> From: Mats Randgaard <matrandg@cisco.com>
> 
> The driver is tested on our hardware and all the implemented features
> works as expected.
> 
> Missing features:
> - CEC support
> - HDCP repeater support
> - IR support
> 
> Signed-off-by: Mats Randgaard <matrandg@cisco.com>
> ---
>  MAINTAINERS                        |    6 +
>  drivers/media/i2c/Kconfig          |    9 +
>  drivers/media/i2c/Makefile         |    1 +
>  drivers/media/i2c/tc358743.c       | 1838 ++++++++++++++++++++++++++++++++++++
>  drivers/media/i2c/tc358743_regs.h  |  680 +++++++++++++
>  include/media/tc358743.h           |   92 ++
>  include/uapi/linux/v4l2-controls.h |    4 +
>  7 files changed, 2630 insertions(+)
>  create mode 100644 drivers/media/i2c/tc358743.c
>  create mode 100644 drivers/media/i2c/tc358743_regs.h
>  create mode 100644 include/media/tc358743.h
> 

<snip>

> +static int tc358743_set_fmt(struct v4l2_subdev *sd,
> +		struct v4l2_subdev_pad_config *cfg,
> +		struct v4l2_subdev_format *format)
> +{
> +	struct tc358743_state *state = to_state(sd);
> +
> +	if (format->pad != 0)
> +		return -EINVAL;
> +
> +	switch (format->format.code) {
> +	case MEDIA_BUS_FMT_RGB888_1X24:
> +	case MEDIA_BUS_FMT_UYVY8_1X16:
> +		state->mbus_fmt_code = format->format.code;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	enable_stream(sd, false);
> +	tc358743_set_pll(sd);
> +	tc358743_set_csi(sd);
> +	tc358743_set_csi_color_space(sd);
> +
> +	return 0;
> +}

I'd rewrite this as follows:

static int tc358743_set_fmt(struct v4l2_subdev *sd,
		struct v4l2_subdev_pad_config *cfg,
		struct v4l2_subdev_format *format)
{
	struct tc358743_state *state = to_state(sd);
	u32 code = format->format.code; /* is overwritten by get_fmt */
	int ret = tc358743_get_fmt(sd, cfg, format);

	format->format.code = code;

	if (ret)
		return ret;

	switch (code) {
	case MEDIA_BUS_FMT_RGB888_1X24:
	case MEDIA_BUS_FMT_UYVY8_1X16:
		break;
	default:
		return -EINVAL;
	}

	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
		return 0;

	enable_stream(sd, false);
	tc358743_set_pll(sd);
	tc358743_set_csi(sd);
	tc358743_set_csi_color_space(sd);

	return 0;
}

The call to tc358743_get_fmt() ensures that all the other fields are
filled in correctly, and the FORMAT_TRY check ensures that calling this
with FORMAT_TRY doesn't actually change anything.

Obviously, set_fmt should be moved after get_fmt so that it can call
the get_fmt function, and like get_fmt it should be moved to the PAD OPS
section.

<snip>

Regards,

	Hans