Re: [PATCH v3 2/2] Add a driver for the NIR-enhanced Mira220 1600x1400 global shutter image sensor.

[Krzysztof Kozlowski <krzk@kernel.org>]

On 01/07/2025 17:15, Philippe Baetens wrote:
> This driver implements 12b, 10b and 8b RAW RGB color format.
> The output datarate is
> 1500Mbit/s, 2 lane. Framerate is up to 90 fps.
> Note: this sensor does not support analog gain.
> 
> Signed-off-by: philippe baetens <philippebaetens@gmail.com>
> ---
> Changes in v3:
> 	- Add people to mailing list
> 	- Improve commit description
> Changes in v2:
> 	- Fix checkpatch warnings

Subject: missing prefixes
Subject: no full stop, this is not a sentence. Look at git log how this
is created.

> ---
>  drivers/media/i2c/Kconfig   |   39 +
>  drivers/media/i2c/Makefile  |    3 +
>  drivers/media/i2c/mira220.c | 1973 +++++++++++++++++++++++++++++++++++
>  3 files changed, 2015 insertions(+)
>  create mode 100644 drivers/media/i2c/mira220.c
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index e68202954..0267f2440 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -127,6 +127,31 @@ config VIDEO_HI847
>            To compile this driver as a module, choose M here: the
>            module will be called hi847.
>  
> +config VIDEO_PONCHA110

What is this?

> +	tristate "ams PONCHA110 sensor support"
> +	depends on I2C && VIDEO_DEV
> +	select MEDIA_CONTROLLER
> +	select VIDEO_V4L2_SUBDEV_API
> +	select V4L2_FWNODE
> +	help
> +	  This is a Video4Linux2 sensor driver for the ams
> +	  PONCHA110 camera.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called poncha110.
> +
> +config VIDEO_PONCHA110COLOR

Hm?

> +	tristate "ams PONCHA110COLOR sensor support"
> +	depends on I2C && VIDEO_DEV
> +	select MEDIA_CONTROLLER
> +	select VIDEO_V4L2_SUBDEV_API
> +	select V4L2_FWNODE
> +	help
> +	  This is a Video4Linux2 sensor driver for the ams
> +	  PONCHA110 camera.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called poncha110.
>  config VIDEO_IMX208
>  	tristate "Sony IMX208 sensor support"
>  	help
> @@ -269,6 +294,20 @@ config VIDEO_IMX415
>  config VIDEO_MAX9271_LIB
>  	tristate
>  
> +config VIDEO_MIRA220
> +	tristate "ams MIRA220 sensor support"
> +	depends on I2C && VIDEO_DEV
> +	select MEDIA_CONTROLLER
> +	select VIDEO_V4L2_SUBDEV_API
> +	select V4L2_CCI_I2C
> +	select V4L2_FWNODE
> +	help
> +	  This is a Video4Linux2 sensor driver for the ams
> +	  MIRA220 camera.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called mira220.
> +
>  config VIDEO_MT9M001
>  	tristate "mt9m001 support"
>  	help
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index 5873d2943..3b5e9d242 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -45,6 +45,8 @@ obj-$(CONFIG_VIDEO_HI556) += hi556.o
>  obj-$(CONFIG_VIDEO_HI846) += hi846.o
>  obj-$(CONFIG_VIDEO_HI847) += hi847.o
>  obj-$(CONFIG_VIDEO_I2C) += video-i2c.o
> +obj-$(CONFIG_VIDEO_PONCHA110)	+= poncha110.o
> +obj-$(CONFIG_VIDEO_PONCHA110COLOR)	+= poncha110color.o

What?


...

> +
> +struct mira220 {

Type declarations go before variable definitions.

> +	struct v4l2_subdev sd;
> +	struct media_pad pad;
> +
> +	struct v4l2_mbus_framefmt fmt;
> +
> +	struct clk *xclk; /* system clock to MIRA220 */
> +	u32 xclk_freq;
> +
> +	struct regulator_bulk_data supplies[MIRA220_NUM_SUPPLIES];
> +
> +	struct v4l2_ctrl_handler ctrl_handler;
> +	struct v4l2_ctrl *pixel_rate;
> +	struct v4l2_ctrl *vflip;
> +	struct v4l2_ctrl *hflip;
> +	struct v4l2_ctrl *vblank;
> +	struct v4l2_ctrl *hblank;
> +	struct v4l2_ctrl *exposure;
> +	struct v4l2_ctrl *gain;
> +
> +	/* Current mode */
> +	const struct mira220_mode *mode;
> +
> +	struct mutex mutex; //comment
> +	/* mutex ensures correct initialization */
> +
> +	struct regmap *regmap;
> +};
> +
> +static inline struct mira220 *to_mira220(struct v4l2_subdev *_sd)
> +{
> +	return container_of(_sd, struct mira220, sd);
> +}
> +
> +/* Power/clock management functions */
> +static int mira220_power_on(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct mira220 *mira220 = to_mira220(sd);
> +	int ret = -EINVAL;
> +
> +	ret = regulator_bulk_enable(MIRA220_NUM_SUPPLIES, mira220->supplies);
> +	if (ret) {
> +		dev_err(&client->dev, "%s: failed to enable regulators\n",
> +			__func__);
> +		goto reg_off;
> +	}
> +	ret = clk_prepare_enable(mira220->xclk);
> +	if (ret) {
> +		dev_err(&client->dev, "%s: failed to enable clock\n", __func__);
> +		goto clk_off;
> +	}
> +	fsleep(MIRA220_XCLR_MIN_DELAY_US);

No. Look at your probe code, this makes no sense.

> +
> +	return 0;
> +
> +clk_off:
> +	clk_disable_unprepare(mira220->xclk);
> +reg_off:
> +	ret = regulator_bulk_disable(MIRA220_NUM_SUPPLIES, mira220->supplies);

So you return 0 on error?
Why are you disabling regulators which were not enabled? This is wrong -
bug.

> +	return ret;
> +}
> +
> +static int mira220_power_off(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct mira220 *mira220 = to_mira220(sd);
> +	(void)mira220;
> +
> +	clk_disable_unprepare(mira220->xclk);
> +	regulator_bulk_disable(MIRA220_NUM_SUPPLIES, mira220->supplies);
> +
> +	return 0;
> +}
> +


...

> +
> +static int mira220_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct mira220 *mira220;
> +	int ret;
> +
> +	mira220 = devm_kzalloc(&client->dev, sizeof(*mira220), GFP_KERNEL);
> +	if (!mira220)
> +		return -ENOMEM;
> +
> +	v4l2_i2c_subdev_init(&mira220->sd, client, &mira220_subdev_ops);
> +	mira220->sd.internal_ops = &mira220_internal_ops;
> +
> +	mira220->regmap = devm_cci_regmap_init_i2c(client, 16);
> +	if (IS_ERR(mira220->regmap))
> +		return dev_err_probe(dev, PTR_ERR(mira220->regmap),
> +				     "failed to initialize CCI\n");

Blank line

> +	/* Get system clock (xclk) */
> +	mira220->xclk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(mira220->xclk)) {
> +		dev_err(dev, "failed to get xclk\n");
> +		return PTR_ERR(mira220->xclk);

Syntax is: return dev_err_probe()

> +	}
> +	mira220->xclk_freq = clk_get_rate(mira220->xclk);
> +	if (mira220->xclk_freq != MIRA220_SUPPORTED_XCLK_FREQ) {
> +		dev_err(dev, "xclk frequency not supported: %d Hz\n",
> +			mira220->xclk_freq);
> +		return -EINVAL;

return dev_err_probe

> +	}
> +
> +	ret = mira220_get_regulators(mira220);
> +	if (ret) {
> +		dev_err(dev, "failed to get regulators\n");
> +		return ret;

Same

> +	}
> +
> +	fsleep(10000);

Uh... Why? This sleep makes no sense at all.

> +
> +	// The sensor must be powered for mira220_identify_module()
> +	// to be able to read the CHIP_ID register
> +
> +	 ret = mira220_power_on(dev);
> +	if (ret)
> +		return ret;

You just slept here!

> +
> +	fsleep(100000);

Oh... so entire probe is delayed by 100 ms + 10 ms (!!!). This is just
some arbitrary and not suspicious delay. Maybe you miss regulator ramps,
but 100 ms here is just wrong.

> +
> +	ret = mira220_identify_module(mira220);
> +	if (ret)
> +		goto error_power_off;
> +
> +	/* Set default mode to max resolution */
> +	mira220->mode = &supported_modes[0];
> +
> +	ret = mira220_init_controls(mira220);
> +	if (ret)
> +		goto error_power_off;
> +
> +	/* Initialize subdev */
> +	mira220->sd.internal_ops = &mira220_internal_ops;
> +	mira220->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> +		V4L2_SUBDEV_FL_HAS_EVENTS;
> +	mira220->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +
> +	/* Initialize source pads */
> +	mira220->pad.flags = MEDIA_PAD_FL_SOURCE;
> +
> +	ret = media_entity_pads_init(&mira220->sd.entity, 1, &mira220->pad);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "failed to init entity pads\n");
> +		goto error_handler_free;
> +	}
> +
> +	mira220->sd.state_lock = mira220->ctrl_handler.lock;
> +	ret = v4l2_subdev_init_finalize(&mira220->sd);
> +	if (ret < 0) {
> +		dev_err_probe(dev, ret, "subdev init error\n");
> +		goto error_media_entity;
> +	}
> +
> +	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;
> +	}
> +
> +	/* Enable runtime PM and turn off the device */
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +	pm_runtime_idle(dev);
> +
> +	return 0;
> +
> +error_subdev_cleanup:
> +	v4l2_subdev_cleanup(&mira220->sd);
> +
> +error_media_entity:
> +	media_entity_cleanup(&mira220->sd.entity);
> +
> +error_handler_free:
> +	mira220_free_controls(mira220);
> +
> +error_power_off:
> +	mira220_power_off(dev);
> +
> +	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);
> +	media_entity_cleanup(&sd->entity);
> +	mira220_free_controls(mira220);
> +
> +	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 */ } };

}; is always in new line.

> +MODULE_DEVICE_TABLE(of, mira220_dt_ids);
> +



Best regards,
Krzysztof