Re: [PATCH 2/5] media: ox05b1s: Add omnivision OX05B1S raw sensor driver

[Krzysztof Kozlowski <krzk@kernel.org>]

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@nxp.com>");
> +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