Re: [PATCH v6 2/2] media: i2c: add os05b10 image sensor driver

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Himanshu,

Thanks for the set. A few comments below...

On Fri, Dec 19, 2025 at 02:15:19PM +0530, Himanshu Bhavani wrote:
> +static int os05b10_disable_streams(struct v4l2_subdev *sd,
> +				   struct v4l2_subdev_state *state,
> +				   u32 pad, u64 streams_mask)
> +{
> +	struct os05b10 *os05b10 = to_os05b10(sd);
> +	int ret;
> +
> +	ret = cci_write(os05b10->cci, OS05B10_REG_CTRL_MODE,
> +			OS05B10_MODE_STANDBY, NULL);
> +	if (ret)
> +		dev_err(os05b10->dev, "%s failed to set stream off\n", __func__);

I'd drop the function name here, it's obvious where this comes from. Same
elsewhere.

> +
> +	pm_runtime_put(os05b10->dev);
> +
> +	return ret;
> +}
> +
> +static int os05b10_init_state(struct v4l2_subdev *sd,
> +			      struct v4l2_subdev_state *state)
> +{
> +	struct v4l2_mbus_framefmt *format;
> +	const struct os05b10_mode *mode;
> +
> +	/* Initialize try_fmt */
> +	format = v4l2_subdev_state_get_format(state, 0);
> +
> +	mode = &supported_modes_10bit[0];
> +	format->code = MEDIA_BUS_FMT_SBGGR10_1X10;
> +
> +	/* Update image pad formate */
> +	format->width = mode->width;
> +	format->height = mode->height;
> +	format->field = V4L2_FIELD_NONE;
> +	format->colorspace = V4L2_COLORSPACE_RAW;
> +	format->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +	format->xfer_func = V4L2_XFER_FUNC_NONE;
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_subdev_video_ops os05b10_video_ops = {
> +	.s_stream = v4l2_subdev_s_stream_helper,
> +};
> +
> +static const struct v4l2_subdev_pad_ops os05b10_pad_ops = {
> +	.enum_mbus_code = os05b10_enum_mbus_code,
> +	.get_fmt = v4l2_subdev_get_fmt,
> +	.set_fmt = os05b10_set_pad_format,
> +	.get_selection = os05b10_get_selection,
> +	.enum_frame_size = os05b10_enum_frame_size,
> +	.enable_streams = os05b10_enable_streams,
> +	.disable_streams = os05b10_disable_streams,
> +};
> +
> +static const struct v4l2_subdev_internal_ops os05b10_internal_ops = {
> +	.init_state = os05b10_init_state,
> +};
> +
> +static const struct v4l2_subdev_ops os05b10_subdev_ops = {
> +	.video = &os05b10_video_ops,
> +	.pad = &os05b10_pad_ops,
> +};
> +
> +static const struct v4l2_ctrl_ops os05b10_ctrl_ops = {
> +	.s_ctrl = os05b10_set_ctrl,
> +};
> +
> +static int os05b10_identify_module(struct os05b10 *os05b10)
> +{
> +	int ret;
> +	u64 val;
> +
> +	ret = cci_read(os05b10->cci, OS05B10_REG_CHIP_ID, &val, NULL);
> +	if (ret)
> +		return dev_err_probe(os05b10->dev, ret,
> +				     "failed to read chip id %x\n",
> +				     OS05B10_CHIP_ID);
> +
> +	if (val != OS05B10_CHIP_ID)
> +		return dev_err_probe(os05b10->dev, -ENODEV,
> +				     "chip id mismatch: %x!=%llx\n",
> +				     OS05B10_CHIP_ID, val);
> +
> +	return 0;
> +}
> +
> +static int os05b10_power_on(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct os05b10 *os05b10 = to_os05b10(sd);
> +	unsigned long delay_us;
> +	int ret;
> +
> +	/* Enable power rails */
> +	ret = regulator_bulk_enable(ARRAY_SIZE(os05b10_supply_name),
> +				    os05b10->supplies);
> +	if (ret) {
> +		dev_err(os05b10->dev, "failed to enable regulators\n");
> +		return ret;
> +	}
> +
> +	/* Enable xclk */
> +	ret = clk_prepare_enable(os05b10->xclk);
> +	if (ret) {
> +		dev_err(os05b10->dev, "failed to enable clock\n");
> +		goto err_regulator_off;
> +	}
> +
> +	gpiod_set_value_cansleep(os05b10->reset_gpio, 0);
> +
> +	/* Delay T1 */
> +	fsleep(5 * USEC_PER_MSEC);
> +
> +	/* Delay T2 (8192 cycles before SCCB/I2C access) */
> +	delay_us = DIV_ROUND_UP(8192, OS05B10_XCLK_FREQ / 1000 / 1000);
> +	usleep_range(delay_us, delay_us * 2);
> +
> +	return 0;
> +
> +err_regulator_off:
> +	regulator_bulk_disable(ARRAY_SIZE(os05b10_supply_name),
> +			       os05b10->supplies);
> +
> +	return ret;
> +}
> +
> +static int os05b10_power_off(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct os05b10 *os05b10 = to_os05b10(sd);
> +
> +	gpiod_set_value_cansleep(os05b10->reset_gpio, 1);
> +
> +	regulator_bulk_disable(ARRAY_SIZE(os05b10_supply_name), os05b10->supplies);

Can you run

	$ ./scripts/checkpatch.pl --strict --max-line-length=80

on the set, please?

> +	clk_disable_unprepare(os05b10->xclk);
> +
> +	return 0;
> +}
> +
> +static int os05b10_parse_endpoint(struct os05b10 *os05b10)
> +{
> +	struct v4l2_fwnode_endpoint bus_cfg = {
> +		.bus_type = V4L2_MBUS_CSI2_DPHY
> +	};
> +	unsigned long link_freq_bitmap;
> +	struct fwnode_handle *ep;
> +	int ret;
> +
> +	ep = fwnode_graph_get_next_endpoint(dev_fwnode(os05b10->dev), NULL);

Please use fwnode-graph_get_endpoint_by_id() instead.

> +	if (!ep) {
> +		dev_err(os05b10->dev, "Failed to get next endpoint\n");
> +		return -ENXIO;
> +	}
> +
> +	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> +	fwnode_handle_put(ep);
> +	if (ret)
> +		return ret;
> +
> +	if (bus_cfg.bus.mipi_csi2.num_data_lanes != 4) {
> +		ret = dev_err_probe(os05b10->dev, -EINVAL,
> +				    "only 4 data lanes are supported\n");
> +		goto error_out;
> +	}
> +
> +	ret = v4l2_link_freq_to_bitmap(os05b10->dev, bus_cfg.link_frequencies,
> +				       bus_cfg.nr_of_link_frequencies,
> +				       link_frequencies,
> +				       ARRAY_SIZE(link_frequencies),
> +				       &link_freq_bitmap);
> +
> +	if (ret)
> +		dev_err(os05b10->dev, "only 600MHz frequency is available\n");
> +
> +error_out:
> +	v4l2_fwnode_endpoint_free(&bus_cfg);
> +
> +	return ret;
> +}

-- 
Kind regards,

Sakari Ailus