| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5cba2560-075c-4cd3-998e-30bd4ebb1d8b@kernel.org>
Date: Wed, 2 Jul 2025 09:13:36 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Philippe Baetens <philippebaetens@...il.com>, mchehab@...nel.org,
robh@...nel.org, conor+dt@...nel.org, krzk+dt@...nel.org
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org
Subject: Re: [PATCH v3 2/2] Add a driver for the NIR-enhanced Mira220
1600x1400 global shutter image sensor.
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@...il.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
Powered by blists - more mailing lists