Hi Sakari,

On 19-06-25 15:27, Sakari Ailus wrote:

Hi Marco,

Thanks for the set.

Looks quite good to me in general; a few minor comments below.

sorry for my very long absence here. The project got stuck but is now
back on track :)

On Wed, Jun 19, 2019 at 05:28:38PM +0200, Marco Felsch wrote:

quoted

+config VIDEO_TC358746
+	tristate "Toshiba TC358746 parallel-CSI2 bridge"
+	depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER
+	select V4L2_FWNODE
+	help
+	  Support for the Toshiba TC358746 PARALLEL to MIPI CSI-2 bridge.
+	  The bridge can work in both directions but currenty only the

"currently"

Okay.

quoted

+MODULE_DESCRIPTION("Toshiba TC358746 Parallel to CSI-2 bridge driver");
+MODULE_AUTHOR("Marco Felsch [off-list ref]");
+MODULE_LICENSE("GPL v2");

These are usually placed at the end.

I will place these lines at the end.

quoted

+
+#define I2C_MAX_XFER_SIZE	(512 + 2)
+#define TC358746_MAX_FIFO_SIZE	512
+#define TC358746_DEF_LINK_FREQ	0
+
+#define TC358746_LINEINIT_MIN_US	110
+#define TC358746_TWAKEUP_MIN_US		1200
+#define TC358746_LPTXTIME_MIN_NS	55
+#define TC358746_TCLKZERO_MIN_NS	305
+#define TC358746_TCLKTRAIL_MIN_NS	65
+#define TC358746_TCLKPOST_MIN_NS	65
+#define TC358746_THSZERO_MIN_NS		150
+#define TC358746_THSTRAIL_MIN_NS	65
+#define TC358746_THSPREPARE_MIN_NS	45
+
+static const struct v4l2_mbus_framefmt tc358746_def_fmt = {
+	.width		= 640,
+	.height		= 480,
+	.code		= MEDIA_BUS_FMT_UYVY8_2X8,
+	.field		= V4L2_FIELD_NONE,
+	.colorspace	= V4L2_COLORSPACE_DEFAULT,
+	.ycbcr_enc	= V4L2_YCBCR_ENC_DEFAULT,
+	.quantization	= V4L2_QUANTIZATION_DEFAULT,
+	.xfer_func	= V4L2_XFER_FUNC_DEFAULT,
+};
+
+struct tc358746_csi_param {
+	unsigned char speed_range;
+	unsigned int  unit_clk_hz;
+	unsigned char unit_clk_mul;
+	unsigned int speed_per_lane; /* bps / lane */
+	unsigned short lane_num;
+	bool is_continuous_clk;
+
+	/* CSI2-TX Parameters */
+	u32 lineinitcnt;
+	u32 lptxtimecnt;
+	u32 twakeupcnt;
+	u32 tclk_preparecnt;
+	u32 tclk_zerocnt;
+	u32 tclk_trailcnt;
+	u32 tclk_postcnt;
+	u32 ths_preparecnt;
+	u32 ths_zerocnt;
+	u32 ths_trailcnt;
+
+	unsigned int csi_hs_lp_hs_ps;
+};

Would it be possible to use struct phy_configure_opts_mipi_dphy and perhaps
phy_mipi_dphy_get_default_config() as well?

Mh.. the meaning is nearly the same since it is a dphy but I would keep
it because:
I extracted the values from the excel sheet provided by Toshiba and
it _can be_ the case that they added a few workarounds for the
limits to the formulas since they do not match 100% with the limits
set by phy_mipi_dphy_get_default_config().

Since those var's represeting regvals too the worst case would be to add
conversion code (struct phy_configure_opts_mipi_dphy to regval) and
slightly timing adaption calculation for the setting to meet the
requirement from Toshiba.

The result of the latter will surely be different though, but still
expected to be valid.

quoted

+
+enum tc358746_csi_direction {
+	TC358746_CSI_RX, /* CSI-in -> Parallel-out */
+	TC358746_CSI_TX  /* Parallel-in -> CSI-out */
+};
+
+struct tc358746_state {
+	struct v4l2_subdev sd;
+	struct i2c_client *i2c_client;
+	struct gpio_desc *reset_gpio;
+
+	/*
+	 * Generic
+	 */
+	struct media_pad pads[2];
+	struct mutex confctl_mutex;
+	struct v4l2_mbus_framefmt fmt;
+	struct v4l2_ctrl_handler hdl;
+	bool fmt_changed;
+	bool test;
+
+	/*
+	 * Chip Clocks
+	 */
+	struct clk  *refclk;
+	/* internal pll */
+	unsigned int pllinclk_hz;
+	u16 pll_prd;
+	u16 pll_fbd;
+
+	/*
+	 * Video Buffer
+	 */
+	u16 vb_fifo; /* The FIFO size is 511x32 */
+
+	/* currently only TC358746_CSI_TX supported */
+	enum tc358746_csi_direction csi_dir;
+
+	/*
+	 * CSI TX
+	 */
+	struct v4l2_ctrl	  *link_freq;
+	struct tc358746_csi_param *link_freq_settings;
+	u64			  *link_frequencies;
+	unsigned int		   link_frequencies_num;
+
+	/*
+	 * Parallel input
+	 */
+	struct v4l2_ctrl *sensor_pclk_ctrl;
+	struct v4l2_ctrl *sensor_hblank_ctrl;
+	unsigned int pclk;
+	unsigned int hblank;
+};
+
+struct tc358746_mbus_fmt {
+	u32 code;
+	u8 bus_width;
+	u8 bpp;		 /* total bpp */
+	u8 pdformat;	 /* peripheral data format */
+	u8 pdataf;	 /* parallel data format option */
+	u8 ppp;		 /* pclk per pixel */
+	bool csitx_only; /* format only in csi-tx mode supported */
+};
+
+/* TODO: Add other formats as required */
+static const struct tc358746_mbus_fmt tc358746_formats[] = {
+	{
+		.code = MEDIA_BUS_FMT_UYVY8_2X8,

Note that on CSI-2 V4L2 uses the one pixel / sample variants. I.e. this
would be MEDIA_BUS_FMT_UYVY8_1X16.

Thanks for the hint I need to (re-)check this.

quoted

+		.bus_width = 8,
+		.bpp = 16,
+		.pdformat = DATAFMT_PDFMT_YCBCRFMT_422_8_BIT,
+		.pdataf = CONFCTL_PDATAF_MODE0,
+		.ppp = 2,
+	}, {
+		.code = MEDIA_BUS_FMT_UYVY8_1X16,
+		.bus_width = 16,
+		.bpp = 16,
+		.pdformat = DATAFMT_PDFMT_YCBCRFMT_422_8_BIT,
+		.pdataf = CONFCTL_PDATAF_MODE1,
+		.ppp = 1,
+	}, {
+		.code = MEDIA_BUS_FMT_YUYV8_1X16,
+		.bus_width = 16,
+		.bpp = 16,
+		.pdformat = DATAFMT_PDFMT_YCBCRFMT_422_8_BIT,
+		.pdataf = CONFCTL_PDATAF_MODE2,
+		.ppp = 1,
+	}, {
+		.code = MEDIA_BUS_FMT_UYVY10_2X10,
+		.bus_width = 10,
+		.bpp = 20,
+		.pdformat = DATAFMT_PDFMT_YCBCRFMT_422_10_BIT,
+		.pdataf = CONFCTL_PDATAF_MODE0, /* don't care */
+		.ppp = 2,
+	}, {
+		/* in datasheet listed as YUV444 */
+		.code = MEDIA_BUS_FMT_GBR888_1X24,
+		.bus_width = 24,
+		.bpp = 24,
+		.pdformat = DATAFMT_PDFMT_YCBCRFMT_444,
+		.pdataf = CONFCTL_PDATAF_MODE0, /* don't care */
+		.ppp = 2,
+		.csitx_only = true,
+	},
+};

...

quoted

+static void i2c_wr(struct v4l2_subdev *sd, u16 reg, u8 *values, u32 n)
+{
+	struct tc358746_state *state = to_state(sd);
+	struct i2c_client *client = state->i2c_client;
+	int err;
+	struct i2c_msg msg;
+	u8 data[I2C_MAX_XFER_SIZE];
+
+	if ((2 + n) > I2C_MAX_XFER_SIZE) {
+		n = I2C_MAX_XFER_SIZE - 2;
+		v4l2_warn(sd, "i2c wr reg=%04x: len=%d is too big!\n",
+			  reg, 2 + n);
+	}
+
+	msg.addr = client->addr;
+	msg.buf = data;
+	msg.len = 2 + n;
+	msg.flags = 0;
+
+	data[0] = reg >> 8;
+	data[1] = reg & 0xff;
+
+	switch (n) {
+	case 1:
+		data[2 + 0] = values[0];
+		break;
+	case 2:
+		data[2 + 0] = values[1];
+		data[2 + 1] = values[0];
+		break;
+	case 4:
+		data[2 + 0] = values[1];
+		data[2 + 1] = values[0];
+		data[2 + 2] = values[3];
+		data[2 + 3] = values[2];
+		break;
+	default:
+		v4l2_info(sd, "unsupported I2C write %d bytes from address 0x%04x\n",
+			  n, reg);

You'd need to return here. Perhaps WARN_ON(1) instead of v4l2_info(), as
this would presumably be a driver bug.

Okay.

quoted

+
+static void tc358746_enable_csi_lanes(struct v4l2_subdev *sd, int enable)
+{
+	struct tc358746_state *state = to_state(sd);
+	struct tc358746_csi_param *csi_setting =
+		tc358746_g_cur_csi_settings(state);
+	unsigned int lanes = csi_setting->lane_num;
+	u32 val = 0;
+
+	if (lanes < 1 || !enable)
+		i2c_wr32(sd, CLW_CNTRL, CLW_CNTRL_CLW_LANEDISABLE_MASK);
+	if (lanes < 1 || !enable)
+		i2c_wr32(sd, D0W_CNTRL, D0W_CNTRL_D0W_LANEDISABLE_MASK);

Please put the two in the same basic block; no need for two conditions that
are the same.

Okay, there is also a of by one.

quoted

+	if (lanes < 2 || !enable)
+		i2c_wr32(sd, D1W_CNTRL, D1W_CNTRL_D1W_LANEDISABLE_MASK);
+	if (lanes < 3 || !enable)
+		i2c_wr32(sd, D2W_CNTRL, D2W_CNTRL_D2W_LANEDISABLE_MASK);
+	if (lanes < 4 || !enable)
+		i2c_wr32(sd, D3W_CNTRL, D2W_CNTRL_D3W_LANEDISABLE_MASK);
+
+	if (lanes > 0 && enable)

Isn't lanes always greater than 0?

You're right.

quoted

+		val |= HSTXVREGEN_CLM_HSTXVREGEN_MASK |
+			HSTXVREGEN_D0M_HSTXVREGEN_MASK;
+	if (lanes > 1 && enable)
+		val |= HSTXVREGEN_D1M_HSTXVREGEN_MASK;
+	if (lanes > 2 && enable)
+		val |= HSTXVREGEN_D2M_HSTXVREGEN_MASK;
+	if (lanes > 3 && enable)
+		val |= HSTXVREGEN_D3M_HSTXVREGEN_MASK;
+
+	i2c_wr32(sd, HSTXVREGEN, val);
+}
+
+static void tc358746_set_csi(struct v4l2_subdev *sd)
+{
+	struct tc358746_state *state = to_state(sd);
+	struct tc358746_csi_param *csi_setting =
+		tc358746_g_cur_csi_settings(state);
+	bool en_continuous_clk = csi_setting->is_continuous_clk;
+	u32 val;
+
+	val = TCLK_HEADERCNT_TCLK_ZEROCNT_SET(csi_setting->tclk_zerocnt) |
+	      TCLK_HEADERCNT_TCLK_PREPARECNT_SET(csi_setting->tclk_preparecnt);
+	i2c_wr32(sd, TCLK_HEADERCNT, val);
+	val = THS_HEADERCNT_THS_ZEROCNT_SET(csi_setting->ths_zerocnt) |
+	      THS_HEADERCNT_THS_PREPARECNT_SET(csi_setting->ths_preparecnt);
+	i2c_wr32(sd, THS_HEADERCNT, val);
+	i2c_wr32(sd, TWAKEUP, csi_setting->twakeupcnt);
+	i2c_wr32(sd, TCLK_POSTCNT, csi_setting->tclk_postcnt);
+	i2c_wr32(sd, THS_TRAILCNT, csi_setting->ths_trailcnt);
+	i2c_wr32(sd, LINEINITCNT, csi_setting->lineinitcnt);
+	i2c_wr32(sd, LPTXTIMECNT, csi_setting->lptxtimecnt);
+	i2c_wr32(sd, TCLK_TRAILCNT, csi_setting->tclk_trailcnt);
+	i2c_wr32(sd, TXOPTIONCNTRL,
+		 en_continuous_clk ? TXOPTIONCNTRL_CONTCLKMODE_MASK : 0);
+
+	if (state->test)
+		tc38764_debug_pattern_80(sd);
+
+	tc358746_dump_csi(sd, csi_setting);
+}
+
+static void tc358746_enable_csi_module(struct v4l2_subdev *sd, int enable)
+{
+	struct tc358746_state *state = to_state(sd);
+	struct tc358746_csi_param *csi_setting =
+		tc358746_g_cur_csi_settings(state);
+	unsigned int lanes = csi_setting->lane_num;
+	u32 val;
+
+	if (!enable)
+		return;
+
+	i2c_wr32(sd, STARTCNTRL, STARTCNTRL_START_MASK);
+	i2c_wr32(sd, CSI_START, CSI_START_STRT_MASK);
+
+	val = CSI_CONTROL_NOL_1_MASK;
+	if (lanes == 2)
+		val = CSI_CONTROL_NOL_2_MASK;
+	else if (lanes == 3)
+		val = CSI_CONTROL_NOL_3_MASK;
+	else if (lanes == 4)
+		val = CSI_CONTROL_NOL_4_MASK;
+
+	val |= CSI_CONTROL_CSI_MODE_MASK | CSI_CONTROL_TXHSMD_MASK;
+	tc358746_wr_csi_control(sd, val);
+}
+
+static void tc358746_set_buffers(struct v4l2_subdev *sd)
+{
+	struct tc358746_state *state = to_state(sd);
+	const struct tc358746_mbus_fmt *tc358746_mbusfmt =
+		tc358746_get_format(state->fmt.code);
+	unsigned int byte_per_line =
+		(state->fmt.width * tc358746_mbusfmt->bpp) / 8;
+
+	i2c_wr16(sd, FIFOCTL, state->vb_fifo);
+	i2c_wr16(sd, WORDCNT, byte_per_line);
+	v4l2_dbg(1, debug, sd, "FIFOCTL 0x%02x: WORDCNT 0x%02x\n",
+		state->vb_fifo, byte_per_line);
+}
+
+/* --------------- CORE OPS --------------- */
+
+static int tc358746_log_status(struct v4l2_subdev *sd)
+{
+	struct tc358746_state *state = to_state(sd);
+	uint16_t sysctl = i2c_rd16(sd, SYSCTL);
+
+	v4l2_info(sd, "-----Chip status-----\n");
+	v4l2_info(sd, "Chip ID: 0x%02lx\n",
+		  (i2c_rd16(sd, CHIPID) & CHIPID_CHIPID_MASK) >> 8);
+	v4l2_info(sd, "Chip revision: 0x%02lx\n",
+		  i2c_rd16(sd, CHIPID) & CHIPID_REVID_MASK);
+	v4l2_info(sd, "Sleep mode: %s\n", sysctl & SYSCTL_SLEEP_MASK ?
+		  "on" : "off");
+
+	v4l2_info(sd, "-----CSI-TX status-----\n");
+	v4l2_info(sd, "Waiting for particular sync signal: %s\n",
+			(i2c_rd16(sd, CSI_STATUS) & CSI_STATUS_S_WSYNC_MASK) ?
+			"yes" : "no");
+	v4l2_info(sd, "Transmit mode: %s\n",
+			(i2c_rd16(sd, CSI_STATUS) & CSI_STATUS_S_TXACT_MASK) ?
+			"yes" : "no");
+	v4l2_info(sd, "Stopped: %s\n",
+			(i2c_rd16(sd, CSI_STATUS) & CSI_STATUS_S_HLT_MASK) ?
+			"yes" : "no");
+	v4l2_info(sd, "Color space: %s\n",
+			state->fmt.code == MEDIA_BUS_FMT_UYVY8_2X8 ?
+			"YCbCr 422 8-bit" : "Unsupported");
+
+	return 0;
+}
+
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+static void tc358746_print_register_map(struct v4l2_subdev *sd)
+{
+	v4l2_info(sd, "0x0000-0x0050: Global Register\n");
+	v4l2_info(sd, "0x0056-0x0070: Rx Control Registers\n");
+	v4l2_info(sd, "0x0080-0x00F8: Rx Status Registers\n");
+	v4l2_info(sd, "0x0100-0x0150: Tx D-PHY Register\n");
+	v4l2_info(sd, "0x0204-0x0238: Tx PPI Register\n");
+	v4l2_info(sd, "0x040c-0x0518: Tx Control Register\n");
+}
+
+static int tc358746_get_reg_size(u16 address)
+{
+	if (address <= 0x00ff)
+		return 2;
+	else if ((address >= 0x0100) && (address <= 0x05FF))
+		return 4;
+	else
+		return 1;
+}
+
+static int tc358746_g_register(struct v4l2_subdev *sd,
+			       struct v4l2_dbg_register *reg)
+{
+	if (reg->reg > 0xffff) {
+		tc358746_print_register_map(sd);
+		return -EINVAL;
+	}
+
+	reg->size = tc358746_get_reg_size(reg->reg);
+
+	reg->val = i2c_rdreg(sd, reg->reg, reg->size);
+
+	return 0;
+}
+
+static int tc358746_s_register(struct v4l2_subdev *sd,
+			       const struct v4l2_dbg_register *reg)
+{
+	if (reg->reg > 0xffff) {
+		tc358746_print_register_map(sd);
+		return -EINVAL;
+	}
+
+	i2c_wrreg(sd, (u16)reg->reg, reg->val,
+			tc358746_get_reg_size(reg->reg));
+
+	return 0;
+}
+#endif
+
+/* --------------- video ops --------------- */
+
+static int tc358746_s_power(struct v4l2_subdev *sd, int on)
+{
+	struct tc358746_state *state = to_state(sd);
+
+	/*
+	 * REF_01:
+	 * Softreset don't reset configuration registers content but is needed

"doesn't"

Okay.

...

quoted

+static int tc358746_s_stream(struct v4l2_subdev *sd, int enable)
+{
+	tc358746_enable_stream(sd, enable);

Could you use tc358746_enable_stream() directly as the s_stream op?

Of course!

Note that you need to call the upstream sub-device's s_stream op from here
as well.

Yes I missed that thanks for the hint. Currently I' asking myself why it
worked without.

...

quoted

+/* --------------- PROBE / REMOVE --------------- */
+
+static int tc358746_set_lane_settings(struct tc358746_state *state,
+				      struct v4l2_fwnode_endpoint *fw)
+{
+	struct v4l2_subdev *sd = &state->sd;
+	int i;

unsigned int ?

Yes.

...

quoted

+static int tc358746_apply_fw(struct tc358746_state *state)
+{
+	struct v4l2_subdev *sd = &state->sd;
+	struct tc358746_csi_param *csi_setting;
+	int err, i;

unsigned int i

Yes.

quoted

+
+	for (i = 0; i < state->link_frequencies_num; i++) {
+		csi_setting = &state->link_freq_settings[i];
+
+		err = tc358746_calculate_csi_txtimings(state, csi_setting);
+		if (err) {
+			v4l2_err(sd, "Failed to calc csi-tx tminings\n");
+			return err;
+		}
+	}
+
+	/*
+	 * Set it to the hw default value. The correct value will be set during
+	 * set_fmt(), since it depends on the pclk and and the resulution.
+	 */
+	state->vb_fifo = 1;
+
+	err = clk_prepare_enable(state->refclk);
+	if (err) {
+		v4l2_err(sd, "Failed to enable clock\n");
+		return err;
+	}

Is there a need to keep the clock running even if the device is not
streaming? Please consider adding runtime PM support. That's not strictly
needed for the patch to be merged though IMO.

It should be disabled at least at remove, which I missed ...
Thanks for covering that. According you question, I think it should be
possible to add runtime PM. I have also pending patches adding this
device as clock provider. In that case it should be untouched till
runtime PM is supported for the device.

quoted

+
+	if (state->reset_gpio)
+		tc358746_gpio_reset(state);
+
+	return 0;
+}
+
+static int tc358746_probe_fw(struct tc358746_state *state)
+{
+	struct device *dev = &state->i2c_client->dev;
+	struct v4l2_subdev *sd = &state->sd;
+	struct v4l2_fwnode_endpoint endpoint = {
+		.bus_type = V4L2_MBUS_CSI2_DPHY,
+	};
+	struct fwnode_handle *fw_node;
+	unsigned int refclk, pllinclk;
+	unsigned char pll_prediv;
+	int ret = -EINVAL;

ret is always assigned elsewhere; no need to initialise.

Yes.

quoted

+
+	/* Parse all clocks */
+	state->refclk = devm_clk_get(dev, NULL);
+	if (IS_ERR(state->refclk)) {
+		if (PTR_ERR(state->refclk) != -EPROBE_DEFER)
+			v4l2_err(sd, "failed to get refclk: %ld\n",
+				PTR_ERR(state->refclk));
+		return PTR_ERR(state->refclk);
+	}
+
+	refclk = clk_get_rate(state->refclk);
+	if (refclk < 6000000 || refclk > 40000000) {
+		v4l2_err(sd, "Invalid refclk %u, valid range: 6-40 MHz\n",
+			 refclk);
+		return -EINVAL;
+	}
+
+	/*
+	 * The PLL input clock is obtained by dividing refclk by pll_prd.
+	 * It must be between 4 MHz and 40 MHz, lower frequency is better.
+	 */
+	pll_prediv = DIV_ROUND_CLOSEST(refclk, 4000000);
+	if (pll_prediv < 1 || pll_prediv > 16) {
+		v4l2_err(sd, "Invalid pll pre-divider value: %d\n", pll_prediv);
+		return -EINVAL;
+	}
+	state->pll_prd = pll_prediv;
+
+	pllinclk = DIV_ROUND_CLOSEST(refclk, pll_prediv);
+	if (pllinclk < 4000000 || pllinclk > 40000000) {
+		v4l2_err(sd, "Invalid pll input clock: %d Hz\n", pllinclk);
+		return -EINVAL;
+	}
+	state->pllinclk_hz = pllinclk;
+
+	/* Now parse the fw-node */
+	fwnode_graph_for_each_endpoint(dev_fwnode(dev), fw_node) {
+		struct fwnode_endpoint fw_ep;
+
+		ret = fwnode_graph_parse_endpoint(fw_node, &fw_ep);
+		if (ret)
+			return -EINVAL;
+
+		/* Get downstream endpoint */
+		if (fw_ep.port == 1)
+			break;

You could use fwnode_graph_get_endpoint_by_id() instead of the loop.

You're right I will change that.

...

quoted

+static int tc358746_async_register(struct v4l2_subdev *sd)
+{
+	unsigned int port = 0;
+
+	return v4l2_async_register_fwnode_subdev(
+		sd, sizeof(struct v4l2_async_subdev), &port, 1,
+		tc358746_parse_endpoint);

Please move this to the caller instead.

Okay.

quoted

+
+}
+
+static const char * const tc358764_test_pattern_menu[] = {
+	"Disabled",
+	"80 Pixel Color Bars",
+};
+
+static int tc358746_probe(struct i2c_client *client)
+{
+	struct tc358746_state *state;
+	struct v4l2_subdev *sd;
+	int err;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
+		return -EIO;
+
+	state = devm_kzalloc(&client->dev, sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return -ENOMEM;
+
+	state->i2c_client = client;

You can use v4l2_get_subdevdata() to obtain the I²C client; no need to
store it separately.

Yep I know now ^^

quoted

+	sd = &state->sd;
+	v4l2_i2c_subdev_init(sd, client, &tc358746_ops);
+	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+
+	err = tc358746_probe_fw(state);
+	if (err)
+		return err;
+
+	err = tc358746_apply_fw(state);
+	if (err)
+		return err;
+
+	if (((i2c_rd16(sd, CHIPID) & CHIPID_CHIPID_MASK) >> 8) != 0x44) {
+		v4l2_info(sd, "not a TC358746 on address 0x%x\n",
+			  client->addr << 1);
+		err = -ENODEV;
+		goto err_clk;
+	}
+
+	v4l2_ctrl_handler_init(&state->hdl, 2);
+
+	v4l2_ctrl_new_std_menu_items(&state->hdl,
+			&tc358764_ctrl_ops, V4L2_CID_TEST_PATTERN,
+			ARRAY_SIZE(tc358764_test_pattern_menu) - 1, 0, 0,
+			tc358764_test_pattern_menu);
+
+	state->link_freq =
+		v4l2_ctrl_new_int_menu(&state->hdl, &tc358764_ctrl_ops,
+				       V4L2_CID_LINK_FREQ,
+				       state->link_frequencies_num - 1,
+				       TC358746_DEF_LINK_FREQ,
+				       state->link_frequencies);
+
+
+	sd->ctrl_handler = &state->hdl;
+	if (state->hdl.error) {
+		err = state->hdl.error;
+		goto err_hdl;
+	}
+
+	state->pads[0].flags = MEDIA_PAD_FL_SINK;
+	state->pads[1].flags = MEDIA_PAD_FL_SOURCE;
+	sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
+	sd->entity.ops = &tc358746_entity_ops;
+	err = media_entity_pads_init(&sd->entity, 2, state->pads);
+	if (err < 0)
+		goto err_hdl;
+
+	mutex_init(&state->confctl_mutex);
+
+	state->fmt = tc358746_def_fmt;
+
+	/* Apply default settings */
+	tc358746_sreset(sd);
+	tc358746_set_buffers(sd);
+	tc358746_set_csi(sd);
+	tc358746_set_csi_color_space(sd);
+	tc358746_sleep_mode(sd, 1);
+	tc358746_set_pll(sd);
+	tc358746_enable_stream(sd, 0);
+
+	err = tc358746_async_register(sd);
+	if (err < 0)
+		goto err_mutex;
+
+	v4l2_info(sd, "%s found @ 0x%x (%s)\n", client->name,
+		  client->addr << 1, client->adapter->name);
+
+	return 0;
+
+err_mutex:
+	mutex_destroy(&state->confctl_mutex);
+err_hdl:
+	media_entity_cleanup(&sd->entity);
+	v4l2_ctrl_handler_free(&state->hdl);
+err_clk:
+	clk_disable_unprepare(state->refclk);
+
+	return err;
+}
+
+static int tc358746_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct tc358746_state *state = to_state(sd);
+
+	v4l2_async_unregister_subdev(sd);
+	v4l2_device_unregister_subdev(sd);
+	mutex_destroy(&state->confctl_mutex);
+	media_entity_cleanup(&sd->entity);
+	v4l2_ctrl_handler_free(&state->hdl);
+
+	return 0;
+}
+
+static const struct of_device_id __maybe_unused tc358746_of_match[] = {
+	{ .compatible = "toshiba,tc358746" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, tc358746_of_match);
+
+static struct i2c_driver tc358746_driver = {
+	.driver = {
+		.name = "tc358746",
+		.of_match_table = of_match_ptr(tc358746_of_match),

No need for of_patch_ptr() here.

Okay.

Thanks for the review :)

Regards,
  Marco