On 28/10/2024 20:06, Mirela Rabulea wrote:
> Add a v4l2 subdevice driver for the Omnivision OX05B1S RGB-IR sensor.
>
> The Omnivision OX05B1S is a 1/2.5-Inch CMOS image sensor with an
> active array size of 2592 x 1944.
>
> The following features are supported for OX05B1S:
> - Manual exposure an gain control support
> - vblank/hblank control support
> - Supported resolution: 2592 x 1944 @ 30fps (SGRBG10)
...
> +
> +static const struct v4l2_subdev_ops ox05b1s_subdev_ops = {
> + .video = &ox05b1s_subdev_video_ops,
> + .pad = &ox05b1s_subdev_pad_ops,
> +};
> +
> +static const struct v4l2_subdev_internal_ops ox05b1s_internal_ops = {
> + .init_state = ox05b1s_init_state,
> +};
> +
> +static void ox05b1s_get_gpios(struct ox05b1s *sensor)
> +{
> + struct device *dev = &sensor->i2c_client->dev;
> +
> + sensor->rst_gpio = devm_gpiod_get_optional(dev, "reset",
> + GPIOD_OUT_HIGH);
> + if (IS_ERR(sensor->rst_gpio))
> + dev_warn(dev, "No sensor reset pin available");
Same comment as for clock further.
> +}
> +
...
> +static int ox05b1s_probe(struct i2c_client *client)
> +{
> + int retval;
> + struct device *dev = &client->dev;
> + struct v4l2_subdev *sd;
> + struct ox05b1s *sensor;
> +
> + sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
> + if (!sensor)
> + return -ENOMEM;
> +
> + sensor->regmap = devm_regmap_init_i2c(client, &ox05b1s_regmap_config);
> + if (IS_ERR(sensor->regmap)) {
> + dev_err(dev, "Failed to allocate sensor register map\n");
Allocation errors never result with error msg. Unless you meant
something else than allocation, but then syntax is return dev_err_probe.
> + return PTR_ERR(sensor->regmap);
> + }
> +
> + sensor->i2c_client = client;
> +
> + sensor->model = of_device_get_match_data(dev);
> +
> + ox05b1s_get_gpios(sensor);
> +
> + sensor->sensor_clk = devm_clk_get(dev, "csi_mclk");
> + if (IS_ERR(sensor->sensor_clk)) {
> + sensor->sensor_clk = NULL;
> + dev_warn(dev, "Sensor csi_mclk is missing, using oscillator from sensor module\n");
Nope, syntax is return dev_err_probe. Why would you warn on probe deferral?
> + }
> +
...
> +
> +module_i2c_driver(ox05b1s_i2c_driver);
> +MODULE_DESCRIPTION("Omnivision OX05B1S MIPI Camera Subdev Driver");
> +MODULE_AUTHOR("Mirela Rabulea <mirela.rabulea@xxxxxxx>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/media/i2c/ox05b1s/ox05b1s_regs_5mp.h b/drivers/media/i2c/ox05b1s/ox05b1s_regs_5mp.h
> new file mode 100644
> index 000000000000..3c34724c1d7e
> --- /dev/null
> +++ b/drivers/media/i2c/ox05b1s/ox05b1s_regs_5mp.h
> @@ -0,0 +1,1160 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * A register configuration for Omnivision OX05B1S raw camera, 2592 x 1944 @30fps BGGR10
> + * Copyright (C) 2024, NXP
> + * Copyright (C) 2024, Omnivision
> + *
> + */
> +#ifndef _OX05B1S_REGS_2592x1944_H_
> +#define _OX05B1S_REGS_2592x1944_H_
> +
> +/* 2592X1944_30FPS_FULL_RGBIr 2592 1944 */
> +static struct ox05b1s_reg ovx5b_init_setting_2592x1944[] = {
How this could be in the header? Why do you need multiple of copies of
it? No, move to the driver.
Best regards,
Krzysztof
