Re: [PATCH v2 2/2] media: i2c: mira016: Add driver for Mira016

Hi Sakari

On Mon, Sep 07, 2026 at 11:50:28AM +0300, Sakari Ailus wrote:

Hi Jacopo,

On Mon, Sep 07, 2026 at 09:45:16AM +0200, Jacopo Mondi wrote:

quoted

quoted

quoted

+}
+
+static inline u32 mira016_nsec_to_lines(struct mira016 *mira016, u32 nsec)
+{
+	return nsec / (mira016_trow_psec(mira016) / HZ_PER_KHZ);
+}
+
+static inline u32 mira016_calc_prate(struct mira016 *mira016, u32 h_tot)
+{
+	/*
+	 * Pixel rate is calculate as the row duration divided by the total line
+	 * length.
+	 *
+	 * pixel time (psec) = t_row(psec) / h_tot
+	 * pixel rate (mbps) = 10^12 / pixel_time
+	 *		     = 10^12 * h_tot / t_row
+	 *		     = h_tot * 10^6 / t_row * 10^6 to avoid overflows
+	 */
+	u32 trow_psec = mira016_trow_psec(mira016);
+
+	return h_tot * HZ_PER_MHZ / trow_psec * HZ_PER_MHZ;

Can this overflow?

I don't think so. I chose to multiply by 10^6 two times before and after
the division to avoid overflows.

Should I re-consider something ?

It was a question. :-) If it can't then it's fine as-is.

quoted

quoted

quoted

+}
+
+static inline u32 mira016_calc_min_vblank(struct mira016 *mira016, u32 y_tot)
+{
+	/*
+	 * See 3.15.2 Frame Rate, equation 4.
+	 *
+	 * TODO: The minimum frame duration has to be expanded if embedded data
+	 * are used.
+	 */
+	u32 trow_nsec = mira016_trow_psec(mira016) / HZ_PER_KHZ;
+	u32 min_duration_nsec =  trow_nsec * (y_tot + 35) + 50 * HZ_PER_KHZ;

What are 35 and 50 here?

No idea, the sensor datasheet doesn't describe those values

Ok.

There's also an extra whitespace there.

Oh thanks

...

quoted

quoted

quoted

+static int mira016_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct mira016 *mira016 =
+		container_of(ctrl->handler, struct mira016, ctrl_handler);
+	struct i2c_client *client = v4l2_get_subdevdata(&mira016->sd);
+	struct v4l2_subdev_state *state;
+	struct v4l2_rect *crop;
+	int ret = 0;
+
+	state = v4l2_subdev_get_locked_active_state(&mira016->sd);
+	crop = v4l2_subdev_state_get_crop(state, 0);
+
+	if (ctrl->id == V4L2_CID_VBLANK) {
+		s32 exposure_max = crop->height + ctrl->val
+				 - MIRA016_FRAME_INTEGRATION_DIFF;
+		s32 exposure_def = min(exposure_max,
+				       mira016->exposure->val);
+
+		ret = __v4l2_ctrl_modify_range(mira016->exposure,
+					       mira016->exposure->minimum,
+					       exposure_max,
+					       mira016->exposure->step,
+					       exposure_def);
+		if (ret)
+			return ret;
+	}
+
+	if (!pm_runtime_get_if_in_use(&client->dev))
+		return 0;
+
+	switch (ctrl->id) {
+	case V4L2_CID_EXPOSURE:
+		ret = mira016_write_exposure_reg(mira016, ctrl->val);
+		break;
+	case V4L2_CID_VBLANK:
+		ret = mira016_write_frame_duration_reg(mira016, state, ctrl->val);
+		break;

Is hblank part of the register lists?

These sensors (there will hopefully be more supported by this driver)
do not have a real horizontal blanking.

Their line length is expressed by a time base multipled by a line
length which doesn't directly depend on the pixel width but rather on
the ADC and PHY timings.

You could expand the line duration by increasing the time base, but I
wouldn't go there and use VBLANK only to control the frame duration.

As you can see the HBLANK control is registered with  fixed value of
0.

How does control frame rate then? If HBLANK is zero, the frame rate is
undefined, isn't it?

Why do you think so ?

The row_timing is the product of the time base (in usec) multiplied by
the row length expressed in time base cycles (see mira016_trow_psec()
and how seq_time_base and row_length are calculated).

VBLANK is still controllable, and you can vary the frame rate by
changing the vblank.

What have am I missing ?

quoted

quoted

quoted

+static int mira016_init_controls(struct mira016 *mira016)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&mira016->sd);
+	struct v4l2_fwnode_device_properties props;
+	struct v4l2_ctrl_handler *ctrl_hdlr;
+	struct v4l2_ctrl *link_freq;
+	struct v4l2_ctrl *hblank;
+	u32 min_exposure_lines;
+	u32 def_exposure;
+	u32 min_vblank;
+	u32 def_vblank;
+	u32 pixel_rate;
+	int ret;
+
+	ctrl_hdlr = &mira016->ctrl_handler;
+	v4l2_ctrl_handler_init(ctrl_hdlr, 12);
+
+	/* By default, PIXEL_RATE is read only */
+	pixel_rate = mira016_calc_prate(mira016, MIRA016_PIXEL_ARRAY_WIDTH);
+	mira016->prate = v4l2_ctrl_new_std(ctrl_hdlr, NULL, V4L2_CID_PIXEL_RATE,
+					   pixel_rate, pixel_rate, 1,
+					   pixel_rate);
+
+	def_vblank = mira016_nsec_to_lines(mira016,
+					   MIRA016_DEFAULT_DURATION_NSEC);
+	def_vblank -= MIRA016_PIXEL_ARRAY_HEIGHT;
+
+	min_vblank = mira016_calc_min_vblank(mira016,
+					     MIRA016_PIXEL_ARRAY_HEIGHT);
+	mira016->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &mira016_ctrl_ops,
+					    V4L2_CID_VBLANK, min_vblank,
+					    MIRA016_MAX_VBLANK, 1,
+					    def_vblank);
+
+	/* Fixed 0 horizontal blanking. */
+	hblank = v4l2_ctrl_new_std(ctrl_hdlr, NULL, V4L2_CID_HBLANK, 0,
+				   0, 1, 0);
+
+	link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, NULL, V4L2_CID_LINK_FREQ,
+					   0, 0, &mira016_link_freqs[0]);
+
+	min_exposure_lines = mira016_nsec_to_lines(mira016,
+						   mira016->timings.min_exposure_time);
+	def_exposure = mira016_nsec_to_lines(mira016,
+					     MIRA016_EXPOSURE_DEF_NSEC);
+	mira016->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &mira016_ctrl_ops,
+					      V4L2_CID_EXPOSURE,
+					      min_exposure_lines,
+					      MIRA016_MAX_EXPOSURE, 1,
+					      def_exposure);
+
+	mira016->gain = v4l2_ctrl_new_std(ctrl_hdlr, &mira016_ctrl_ops,
+					  V4L2_CID_ANALOGUE_GAIN,
+					  1, 1, 1, 1);
+
+	/*
+	 * Changing VFLIP requires re-programming the top point, hence we
+	 * program flips along with the ROI windows at enable_streams time. As
+	 * we grab the flip controls there, there's no need to handle the two
+	 * controls while streaming.
+	 */
+	mira016->hflip = v4l2_ctrl_new_std(ctrl_hdlr, NULL,
+					   V4L2_CID_HFLIP, 0, 1, 1, 0);
+
+	mira016->vflip = v4l2_ctrl_new_std(ctrl_hdlr, NULL,
+					   V4L2_CID_VFLIP, 0, 1, 1, 0);
+
+	v4l2_fwnode_device_parse(&client->dev, &props);

v4l2_fwnode_device_parse() can return an error and I'd check for it. I'd
however do that before initialising the control handler as it simplifies
error handling.

Interesting idea. I still see most drivers calling
v4l2_fwnode_device_parse() and v4l2_ctrl_new_fwnode_properties() in
sequence at the end of the controls initialization, but doing the
parsing at the beginning of the function is certainly a good idea.

Yes, the ones you've been looking at are old drivers. :-)

quoted

quoted

quoted

+		{ 16, 21 }, { 17, 26 }, { 18, 29 }, { 19, 30 }, { 20, 15 },
+		{ 21, 23 }, { 22, 27 }, { 23, 13 }, { 24, 22 }, { 25, 11 },
+		{ 26, 5 }, { 27, 18 }, { 28, 25 }, { 29, 12 },
+		{ 30, 6 }, { 31, 3 }, { 32, 1 }
+	};
+
+	for (unsigned int i = 0; i < ARRAY_SIZE(pll_n_lut); ++i) {
+		if (pll_n_lut[i].n != pll_n)
+			continue;
+
+		return pll_n_lut[i].pll_n;
+	}
+
+	return 0;
+}
+
+static u8 mira016_m_to_pll_m(u32 pll_m)
+{
+	/* Table 12: Lookup table for “M to PLL_DIV_M” mapping */
+	static const struct pll_m_div {
+		u8 m_min;
+		u8 m_max;
+		u8 pll_m_min;
+		u8 pll_m_max;
+	} pll_m_lut[] = {
+		{ 16, 31, 224, 239 }, { 32, 63, 192, 233 },
+		{ 64, 127, 128, 191 }, { 128, 255, 0, 127 },
+	};
+
+	for (unsigned int i = 0; i < ARRAY_SIZE(pll_m_lut); ++i) {
+		const struct pll_m_div *p = &pll_m_lut[i];
+
+		if (pll_m > p->m_max)
+			continue;
+
+		return p->pll_m_min + pll_m - p->m_min;
+	}
+
+	return 0;
+}
+
+static void mira016_pll_calc(struct mira016 *mira016)

It's nice to have a PLL calculator for this. Thank you! :-)

I also considered using the CCS PLL calculator, but this PLL is so
simple it felt like an overkill

If the CCS PLL calculator fits for the job, I'd just use it. If it does a
little more, it's possible to use the same value for minimum and maximum
for the relevant parameters.

quoted

quoted

quoted

+static int mira016_parse_endpoint(struct device *dev, struct mira016 *mira016)
+{
+	struct fwnode_handle *endpoint __free(fwnode_handle) = NULL;
+	struct v4l2_fwnode_endpoint ep_cfg = {
+		.bus_type = V4L2_MBUS_CSI2_DPHY
+	};
+
+	endpoint = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0, 0);
+	if (v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep_cfg))
+		return dev_err_probe(dev, -EINVAL, "Failed to parse endpoint\n");

Don't mask error codes! Just return the error code returned by
v4l2_fwnode_endpoint_alloc_parse().

With PTR_ERR() I presume

v4l2_fwnode_endpoint_alloc_parse() returns an integer.

Yeah I just noticed.

quoted

quoted

quoted

+static int mira016_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct mira016 *mira016;
+	int ret;
+
+	mira016 = devm_kzalloc(&client->dev, sizeof(*mira016), GFP_KERNEL);
+	if (!mira016)
+		return -ENOMEM;
+
+	mira016->dev = &client->dev;
+
+	v4l2_i2c_subdev_init(&mira016->sd, client, &mira016_subdev_ops);
+
+	mira016->regmap = devm_cci_regmap_init_i2c(client, 16);
+	if (IS_ERR(mira016->regmap))
+		return dev_err_probe(dev, PTR_ERR(mira016->regmap),
+				     "failed to initialize CCI\n");
+
+	mira016->xclk = devm_v4l2_sensor_clk_get(dev, NULL);
+	if (IS_ERR(mira016->xclk))
+		return dev_err_probe(dev, PTR_ERR(mira016->xclk),
+				     "failed to get xclk\n");
+
+	mira016->xclk_freq = clk_get_rate(mira016->xclk);
+	if (mira016_validate_xclk_freq(mira016)) {
+		dev_err(dev, "xclk frequency not supported: %d Hz\n",
+			mira016->xclk_freq);
+		return -EINVAL;
+	}
+
+	ret = mira016_get_regulators(mira016);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to get regulators\n");
+
+	mira016->reset_gpio = devm_gpiod_get_optional(dev, "reset",
+						      GPIOD_OUT_HIGH);
+	if (IS_ERR(mira016->reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(mira016->reset_gpio),
+				     "failed to get reset gpio\n");
+
+	ret = mira016_parse_endpoint(dev, mira016);
+	if (ret)
+		return ret;

Parsing the endpoint should be done as soon as you can before acquiring
resources related to power management, for instance. -EPROBE_DEFER may well
be returned in some cases.

I can probably do that even before calling v4l2_i2c_subdev_init()

Sounds good.

quoted

quoted

quoted

+static void mira016_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct mira016 *mira016 = to_mira016(sd);
+
+	v4l2_ctrl_handler_free(mira016->sd.ctrl_handler);
+
+	v4l2_async_unregister_subdev(sd);
+	v4l2_subdev_cleanup(&mira016->sd);
+	media_entity_cleanup(&sd->entity);
+
+	pm_runtime_disable(&client->dev);
+	if (!pm_runtime_status_suspended(&client->dev))
+		mira016_power_off(&client->dev);
+	pm_runtime_set_suspended(&client->dev);

pm_runtime_dont_use_autosuspend()?

Ack

Thanks for the review!

You're welcome! :-)

Thanks
  j

--
Regards,

Sakari Ailus