Re: [PATCH 2/2] media: i2c: ov13858: Add device tree support

[Sakari Ailus]

Hi George,

On Thu, Oct 15, 2026 at 10:48:20PM +0100, George Neacsu via B4 Relay wrote:
> From: George Neacsu <geocausa@xxxxxxxxx>
> 
> The rear camera of the Surface Pro 11 with a Snapdragon X Elite is an
> OV13858 on a four-lane D-PHY link, described in the device tree. Add an
> OF match table.
> 
> Parse the endpoint as other sensor drivers that serve both ACPI and DT
> do. Wait for the endpoint if a bridge driver has not created it yet,
> require four data lanes, and require both link frequencies, since the
> modes switch between 540 and 270 MHz. The IPU bridge entry for OVTID858
> lists both, so an ACPI machine with that entry probes as before. Without
> the entry the sensor now waits for the bridge instead of probing into an
> empty graph.
> 
> Tested on the Surface Pro 11 (X1E80100), with an out-of-tree CAMSS
> capturing raw frames over CSIPHY1, CSID0 and an RDI interface:
> 4224x3136 at 29.95 fps at 540 MHz, the colour-bar test pattern intact,
> and a scene captures.

Please describe what the patch does here. Nearly everything above should go
to the cover letter.

> 
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: George Neacsu <geocausa@xxxxxxxxx>
> ---
>  drivers/media/i2c/ov13858.c | 62 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov13858.c b/drivers/media/i2c/ov13858.c
> index 83eefba..587ee38 100644
> --- a/drivers/media/i2c/ov13858.c
> +++ b/drivers/media/i2c/ov13858.c
> @@ -1713,6 +1713,57 @@ static int ov13858_power_off(struct device *dev)
>  static DEFINE_RUNTIME_DEV_PM_OPS(ov13858_pm_ops, ov13858_power_off,
>  				 ov13858_power_on, NULL);
>  
> +static int ov13858_check_hwcfg(struct device *dev)
> +{
> +	struct v4l2_fwnode_endpoint bus_cfg = {
> +		.bus_type = V4L2_MBUS_CSI2_DPHY
> +	};
> +	unsigned long link_freq_bitmap;
> +	struct fwnode_handle *ep;
> +	int ret;
> +
> +	/*
> +	 * Sometimes the fwnode graph is initialized by the bridge driver.
> +	 * Bridge drivers doing this also add sensor properties, wait for this.
> +	 */
> +	ep = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
> +	if (!ep)

You can omit this check -- v4l2_fwnode_endpoint_alloc_parse() does it.

> +		return dev_err_probe(dev, -EPROBE_DEFER,
> +				     "waiting for fwnode graph endpoint\n");
> +
> +	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> +	fwnode_handle_put(ep);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "parsing endpoint failed\n");
> +
> +	if (bus_cfg.bus.mipi_csi2.num_data_lanes != 4) {
> +		ret = dev_err_probe(dev, -EINVAL,
> +				    "%u data lanes are not supported, only 4\n",
> +				    bus_cfg.bus.mipi_csi2.num_data_lanes);
> +		goto out;
> +	}
> +
> +	/* The modes use both link frequencies, so both have to be allowed. */
> +	ret = v4l2_link_freq_to_bitmap(dev, bus_cfg.link_frequencies,
> +				       bus_cfg.nr_of_link_frequencies,
> +				       link_freq_menu_items,
> +				       ARRAY_SIZE(link_freq_menu_items),
> +				       &link_freq_bitmap);
> +	if (ret)
> +		goto out;
> +
> +	if (link_freq_bitmap != GENMASK(ARRAY_SIZE(link_freq_menu_items) - 1, 0))
> +		ret = dev_err_probe(dev, -EINVAL,
> +				    "link frequencies %lld and %lld are both needed\n",
> +				    link_freq_menu_items[1]);
> +
> +out:
> +	v4l2_fwnode_endpoint_free(&bus_cfg);
> +
> +	return ret;
> +}
> +
>  static int ov13858_probe(struct i2c_client *client)
>  {
>  	struct ov13858 *ov13858;
> @@ -1725,6 +1776,10 @@ static int ov13858_probe(struct i2c_client *client)
>  
>  	ov13858->dev = &client->dev;
>  
> +	ret = ov13858_check_hwcfg(ov13858->dev);
> +	if (ret)
> +		return ret;
> +
>  	ov13858->clk = devm_v4l2_sensor_clk_get(ov13858->dev, NULL);
>  	if (IS_ERR(ov13858->clk))
>  		return dev_err_probe(ov13858->dev, PTR_ERR(ov13858->clk),
> @@ -1845,10 +1900,17 @@ static const struct acpi_device_id ov13858_acpi_ids[] = {
>  MODULE_DEVICE_TABLE(acpi, ov13858_acpi_ids);
>  #endif
>  
> +static const struct of_device_id ov13858_of_match[] = {
> +	{ .compatible = "ovti,ov13858" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ov13858_of_match);
> +
>  static struct i2c_driver ov13858_i2c_driver = {
>  	.driver = {
>  		.name = "ov13858",
>  		.acpi_match_table = ACPI_PTR(ov13858_acpi_ids),
> +		.of_match_table = ov13858_of_match,
>  		.pm = pm_ptr(&ov13858_pm_ops),
>  	},
>  	.probe = ov13858_probe,
> 

-- 
Regards,

Sakari Ailus