Re: [PATCH v10 2/2] media: i2c: Add driver for AMS-OSRAM Mira220

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

Hi Jacopo,

On Fri, Jul 31, 2026 at 04:15:53PM +0200, Jacopo Mondi wrote:

...

> > > +	/* Sensor control mode */
> > > +	{ CCI_REG8(0x0043), 0x0 }, //  Sensor Control Mode.SLEEP_POWER_MODE(0)
> > > +	{ CCI_REG8(0x0043), 0x0 }, //  Sensor Control Mode.IDLE_POWER_MODE(0)
> > > +	{ CCI_REG8(0x0043), 0x4 }, //  Sensor Control Mode.SYSTEM_CLOCK_ENABLE(0)
> > > +	{ CCI_REG8(0x0043), 0xC }, //  Sensor Control Mode.SRAM_CLOCK_ENABLE(0)
> > > +	{ CCI_REG8(0x1001), 0x41 }, //  Sensor Control Mode.EXT_EVENT_SEL(0)
> > > +	{ CCI_REG8(0x10f2), 0x1 }, //  Sensor Control Mode.NB_OF_FRAMES_A(0)
> > > +	{ CCI_REG8(0x10f3), 0x0 }, //  Sensor Control Mode.NB_OF_FRAMES_A(1)
> > > +	{ CCI_REG8(0x0012), 0x0 }, //  IO Drive Strength.DIG_DRIVE_STRENGTH(0)
> > > +	{ CCI_REG8(0x0012), 0x0 }, //  IO Drive Strength.CCI_DRIVE_STRENGTH(0)
> > > +	{ CCI_REG8(0x1001), 0x41 }, //  Readout && Exposure.EXT_EXP_PW_SEL(0)
> > > +	{ CCI_REG8(0x10d0), 0x0 }, //  Readout && Exposure.EXT_EXP_PW_DELAY(0)
> > > +	{ CCI_REG8(0x10d1), 0x0 }, //  Readout && Exposure.EXT_EXP_PW_DELAY(1)
> > > +	/* MIPI */
> > > +	{ CCI_REG8(0x6006), 0x0 }, //  MIPI.TX_CTRL_EN(0)
> > > +	{ CCI_REG8(0x5004), 0x1 }, //  MIPI.datarate
> > > +	{ CCI_REG8(0x5086), 0x2 }, //  MIPI.datarate
> > > +	{ CCI_REG8(0x5087), 0x4e }, //  MIPI.datarate
> > > +	{ CCI_REG8(0x5088), 0x0 }, //  MIPI.datarate
> > > +	{ CCI_REG8(0x5090), 0x0 }, //  MIPI.datarate
> >
> > How many of these registers are actually 16 or 32 bits? Does the sensor
> > support wider than single-octet writes -- few don't?
> 
> The sensor is said to support both single read/write and burst read/write
> sequences.
> 
> There is room for improvements here, but as the startup latency is not
> a concern for the time being, I would leave this as an exercize for
> later

Related to the previous comment, it'd be nice to have human-readable names
also for these registers. I'm fine with that if you promise to address this
later. :-)

...

> > > +	ret = pm_runtime_resume_and_get(&client->dev);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	/*
> > > +	 * Apply default values of current mode. Stop streaming before
> > > +	 * uploading register sequence.
> > > +	 */
> > > +	ret = mira220_write_stop_streaming_regs(mira220);
> > > +	if (ret)
> > > +		goto err_rpm_put;
> > > +
> > > +	ret = cci_multi_reg_write(mira220->regmap, mira220_init_reg_list,
> > > +				  ARRAY_SIZE(mira220_init_reg_list), NULL);
> > > +	if (ret)
> > > +		goto err_rpm_put;
> > > +
> > > +	ret = mira220_otp_restore(mira220);
> > > +	if (ret)
> > > +		goto err_rpm_put;
> > > +
> > > +	ret = mira220_set_bus_config(mira220);
> > > +	if (ret)
> > > +		goto err_rpm_put;
> > > +
> > > +	ret = mira220_set_framefmt(mira220, state);
> > > +	if (ret)
> > > +		goto err_rpm_put;
> >
> > Instead of a series of ifs and gotos, you could do
> >
> > 	if (!ret)
> > 		...;
> >
> > Up to you.
> >
> 
> 	ret = pm_runtime_resume_and_get(&client->dev);
> 	if (ret < 0)
> 		return ret;
> 
> 	/*
> 	 * Apply default values of current mode. Stop streaming before
> 	 * uploading register sequence.
> 	 */
> 	if (!ret)
> 		ret = mira220_write_stop_streaming_regs(mira220);
> 	if (!ret)
> 		ret = cci_multi_reg_write(mira220->regmap, mira220_init_reg_list,
> 					  ARRAY_SIZE(mira220_init_reg_list), NULL);
> 	if (!ret)
> 		ret = mira220_otp_restore(mira220);
> 	if (!ret)
> 		ret = mira220_set_bus_config(mira220);
> 	if (!ret)
> 		ret = mira220_set_framefmt(mira220, state);
> 
> 	/* Apply customized values from user */
> 	if (!ret)
> 		ret = __v4l2_ctrl_handler_setup(mira220->sd.ctrl_handler);
> 	if (!ret)
> 		ret = mira220_write_start_streaming_regs(mira220);
> 
> 	if (ret)
> 		goto err_rpm_put;
> 
> 	/* vflip and hflip cannot change during streaming */
> 	__v4l2_ctrl_grab(mira220->hflip, true);
> 	__v4l2_ctrl_grab(mira220->vflip, true);
> 
> 	return 0;
> 
> Looks a bit unusual, but I think I can do that

Such a pattern is used in quite a few sensor drivers.

...

> > > +	/* By default, PIXEL_RATE is read only */
> > > +	v4l2_ctrl_new_std(ctrl_hdlr, &mira220_ctrl_ops, V4L2_CID_PIXEL_RATE,
> > > +			  MIRA220_PIXEL_RATE, MIRA220_PIXEL_RATE, 1,
> > > +			  MIRA220_PIXEL_RATE);
> > > +
> > > +	min_vblank = mira220_calc_min_vblank(mira220);
> > > +	mira220->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &mira220_ctrl_ops,
> > > +					    V4L2_CID_VBLANK,
> > > +					    min_vblank, MIRA220_MAX_VBLANK, 1,
> > > +					    min_vblank);
> > > +
> > > +	ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr, NULL, V4L2_CID_LINK_FREQ,
> > > +				      0, 0, &mira220_link_freqs[0]);
> > > +	if (ctrl)
> > > +		ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > +
> > > +	/*
> > > +	 * Scale hblank according to the number of enabled data lanes to match
> > > +	 * row_length.
> > > +	 */
> > > +	hblank_val = MIRA220_LLP_1600x1400_304 * (2 / mira220->lanes)
> > > +		   - MIRA220_PIXEL_ARRAY_WIDTH;
> > > +	ctrl = v4l2_ctrl_new_std(ctrl_hdlr, NULL, V4L2_CID_HBLANK, hblank_val,
> > > +				 hblank_val, 1, hblank_val);
> > > +	if (ctrl)
> > > +		ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > +
> > > +	/* Max exposure is determined by vblank + vsize and Tglob. */
> > > +	max_exposure = mira220_calc_exposure(mira220,
> > > +					     MIRA220_PIXEL_ARRAY_HEIGHT,
> > > +					     min_vblank);
> > > +
> > > +	mira220->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &mira220_ctrl_ops,
> > > +					      V4L2_CID_EXPOSURE,
> > > +					      MIRA220_EXPOSURE_MIN,
> > > +					      max_exposure, 1,
> > > +					      MIRA220_DEFAULT_EXPOSURE);
> > > +
> > > +	v4l2_ctrl_new_std(ctrl_hdlr, NULL, V4L2_CID_ANALOGUE_GAIN,
> > > +			  MIRA220_ANALOG_GAIN_MIN, MIRA220_ANALOG_GAIN_MAX,
> > > +			  MIRA220_ANALOG_GAIN_STEP,
> > > +			  MIRA220_ANALOG_GAIN_DEFAULT);
> > > +
> > > +	mira220->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &mira220_ctrl_ops,
> > > +					   V4L2_CID_HFLIP, 0, 1, 1, 0);
> > > +	if (mira220->hflip)
> > > +		mira220->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> >
> > You can omit these checks if you move setting the control flags after
> 
> ah yes
> 
> > checking for the handler's error state. I'd use different variables for the
> > hblank and vblank controls for this reason.
> 
> vblank is writable and stored in the driver structure
> 
> link_freq and hblank are RO and re-use the local 'ctrl' variable.

I indeed meant link_freq indeed, not vblank.

> 
> if I move everything below, I'll have to use 2 different variables for
> sure

Yes, that'd be nice.

...

> > > +	ret = v4l2_async_register_subdev_sensor(&mira220->sd);
> > > +	if (ret < 0) {
> > > +		dev_err_probe(dev, ret,
> > > +			      "failed to register sensor sub-device\n");
> > > +		goto error_subdev_cleanup;
> > > +	}
> > > +
> > > +	pm_runtime_idle(dev);
> >
> > You probably want to call pm_runtime_idle() after setting autosuspend
> > delay.
> >
> To be honest I copied this from imx219 but I can certainly change it.

That could be fixed as well. I'll write a patch for it. :-)

> 
> 
> 
> > > +	pm_runtime_set_autosuspend_delay(dev, 1000);
> > > +	pm_runtime_use_autosuspend(dev);
> > > +
> > > +	return 0;
> > > +
> > > +error_subdev_cleanup:
> > > +	v4l2_subdev_cleanup(&mira220->sd);
> > > +error_media_entity:
> > > +	media_entity_cleanup(&mira220->sd.entity);
> > > +error_handler_free:
> > > +	v4l2_ctrl_handler_free(mira220->sd.ctrl_handler);
> > > +error_power_off:
> > > +	pm_runtime_disable(dev);
> > > +	mira220_power_off(dev);
> > > +	pm_runtime_set_suspended(dev);
> >
> > A newline here perhaps?
> >
> 
> Usually I like to keep error path returns compact
> 
> Thanks, I'll send a new version soon.
> It's mostly minor stuff, do you think I can get this in for this cycle
> (a bit last minute, I know)
> 
> Thanks
>   j
> 
> 
> > > +	return ret;
> > > +}
> > > +
> > > +static void mira220_remove(struct i2c_client *client)
> > > +{
> > > +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > +	struct mira220 *mira220 = to_mira220(sd);
> > > +
> > > +	v4l2_async_unregister_subdev(sd);
> > > +	v4l2_subdev_cleanup(&mira220->sd);
> > > +	media_entity_cleanup(&sd->entity);
> > > +
> > > +	v4l2_ctrl_handler_free(mira220->sd.ctrl_handler);
> > > +
> > > +	pm_runtime_disable(&client->dev);
> > > +	if (!pm_runtime_status_suspended(&client->dev))
> > > +		mira220_power_off(&client->dev);
> > > +	pm_runtime_set_suspended(&client->dev);
> > > +}
> > > +
> > > +static const struct dev_pm_ops mira220_pm_ops = {
> > > +	SET_RUNTIME_PM_OPS(mira220_power_off, mira220_power_on, NULL)
> > > +};
> > > +
> > > +static const struct of_device_id mira220_dt_ids[] = {
> > > +	{ .compatible = "ams,mira220" },
> > > +	{ /* sentinel */ }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, mira220_dt_ids);
> > > +
> > > +static struct i2c_driver mira220_i2c_driver = {
> > > +	.driver = {
> > > +		.name = "mira220",
> > > +		.of_match_table	= mira220_dt_ids,
> > > +		.pm = pm_ptr(&mira220_pm_ops),

If CONFIG_PM isn't defined, mira220_pm_ops needs to use __maybe_unused. I'd
just drop pm_ops() here.

-- 
Regards,

Sakari Ailus