Message-ID: <3e47eadc-59b7-4730-89bc-b18e9483fca2@wanadoo.fr>
Date: Fri, 24 Jan 2025 07:56:20 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Mirela Rabulea <mirela.rabulea@....com>, mchehab@...nel.org,
 sakari.ailus@...ux.intel.com, hverkuil-cisco@...all.nl,
 laurent.pinchart+renesas@...asonboard.com, robh@...nel.org,
 krzk+dt@...nel.org, bryan.odonoghue@...aro.org, laurentiu.palcu@....com,
 robert.chiras@....com
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
 LnxRevLi@....com, kieran.bingham@...asonboard.com, hdegoede@...hat.com,
 dave.stevenson@...pberrypi.com, mike.rudenko@...il.com,
 alain.volmat@...s.st.com, devicetree@...r.kernel.org, conor+dt@...nel.org,
 alexander.stein@...tq-group.com, umang.jain@...asonboard.com,
 zhi.mao@...iatek.com, festevam@...x.de, julien.vuillaumier@....com,
 alice.yuan@....com
Subject: Re: [PATCH v3 2/4] media: ox05b1s: Add omnivision OX05B1S raw sensor
 driver

Le 24/01/2025 à 01:12, Mirela Rabulea a écrit :
> 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)
> 
> Signed-off-by: Mirela Rabulea <mirela.rabulea@....com>
> ---

...

> +static int ox05b1s_power_on(struct ox05b1s *sensor)
> +{
> +	struct device *dev = &sensor->i2c_client->dev;
> +	int ret = 0;

No need to init.

> +
> +	ret = regulator_bulk_enable(OX05B1S_NUM_SUPPLIES, sensor->supplies);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable regulators\n");
> +		return ret;
> +	}
> +
> +	/* get out of powerdown and reset */
> +	gpiod_set_value_cansleep(sensor->rst_gpio, 0);
> +
> +	ret = clk_prepare_enable(sensor->sensor_clk);
> +	if (ret < 0) {
> +		dev_err(dev, "Enable sensor clk fail ret=%d\n", ret);
> +		goto reg_off;
> +	}
> +
> +	/* with XVCLK@...Hz, t2 = 6ms required delay for ox05b1s before first SCCB transaction */
> +	fsleep(6000);
> +
> +	return ret;

return 0;

> +
> +reg_off:
> +	regulator_bulk_disable(OX05B1S_NUM_SUPPLIES, sensor->supplies);
> +
> +	return ret;
> +}

...

> +/* needs sensor lock and power on */
> +static int ox05b1s_apply_current_mode(struct ox05b1s *sensor)
> +{
> +	struct device *dev = &sensor->i2c_client->dev;
> +	struct ox05b1s_reglist *reg_data = sensor->mode->reg_data;
> +	u32 w = sensor->mode->width;
> +	u32 h = sensor->mode->height;
> +	int ret = 0;
> +
> +	cci_write(sensor->regmap, OX05B1S_REG_SW_RST, 0x01, NULL);
> +
> +	while (reg_data->regs) {
> +		ret = ox05b1s_write_reg_array(sensor, reg_data->regs);
> +		if (ret)
> +			goto out;
> +		reg_data++;
> +	}
> +
> +	cci_write(sensor->regmap, OX05B1S_REG_X_OUTPUT_SIZE, w, &ret);
> +	cci_write(sensor->regmap, OX05B1S_REG_Y_OUTPUT_SIZE, h, &ret);
> +	if (ret)
> +		goto out;
> +
> +	/* setup handler will write actual controls into sensor registers */
> +	ret =  __v4l2_ctrl_handler_setup(&sensor->ctrls.handler);

2 spaces after '='

> +
> +out:
> +	if (ret < 0)
> +		dev_err(dev, "Failed to apply mode %dx%d,bpp=%d\n", w, h,
> +			sensor->mode->bpp);
> +
> +	return ret;
> +}

...

> +static int ox05b1s_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> +				  struct v4l2_mbus_frame_desc *fd)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	struct ox05b1s *sensor = client_to_ox05b1s(client);
> +
> +	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
> +	fd->num_entries = 1;
> +
> +	/* get sensor current code*/

Missing space before */

> +	mutex_lock(&sensor->lock);
> +	fd->entry[0].pixelcode = sensor->mode->code;
> +	mutex_unlock(&sensor->lock);
> +
> +	fd->entry[0].bus.csi2.vc = 0;
> +	fd->entry[0].bus.csi2.dt = ox05b1s_code2dt(fd->entry[0].pixelcode);
> +
> +	return 0;
> +}

...

> +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");

Missing ending \n

> +}
> +
> +static int ox05b1s_get_regulators(struct ox05b1s *sensor)
> +{
> +	struct device *dev = &sensor->i2c_client->dev;
> +	unsigned int i;
> +
> +	for (i = 0; i < OX05B1S_NUM_SUPPLIES; i++)
> +		sensor->supplies[i].supply = ox05b1s_supply_name[i];
> +
> +	return devm_regulator_bulk_get(dev, OX05B1S_NUM_SUPPLIES, sensor->supplies);
> +}
> +
> +static int ox05b1s_read_chip_id(struct ox05b1s *sensor)
> +{
> +	struct device *dev = &sensor->i2c_client->dev;
> +	u64 chip_id = 0;
> +	char *camera_name;
> +	int ret = 0;

No need to init.

> +
> +	ret = cci_read(sensor->regmap, OX05B1S_REG_CHIP_ID, &chip_id, NULL);
> +	if (ret) {
> +		dev_err(dev, "Camera chip_id read error\n");
> +		return -ENODEV;
> +	}
> +
> +	switch (chip_id) {
> +	case 0x580542:
> +		camera_name = "ox05b1s";
> +		break;
> +	default:
> +		camera_name = "unknown";
> +		break;
> +	}
> +
> +	if (chip_id == sensor->model->chip_id) {
> +		dev_dbg(dev, "Camera %s detected, chip_id=%llx\n", camera_name, chip_id);
> +	} else {
> +		dev_err(dev, "Detected %s camera (chip_id=%llx), but expected %s (chip_id=%x)\n",
> +			camera_name, chip_id, sensor->model->name, sensor->model->chip_id);
> +		ret = -ENODEV;

Could be return -ENODEV;
and return 0; below, to ease reading.

> +	}
> +
> +	return ret;
> +}
> +
> +static int ox05b1s_probe(struct i2c_client *client)
> +{
> +	int retval;

Using just 'ret' would be slighly shorter and more consistent with other 
functions above.

> +	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_cci_regmap_init_i2c(client, 16);
> +	if (IS_ERR(sensor->regmap))
> +		return PTR_ERR(sensor->regmap);
> +
> +	sensor->i2c_client = client;
> +
> +	sensor->model = of_device_get_match_data(dev);
> +
> +	ox05b1s_get_gpios(sensor);
> +
> +	/* Get system clock, xvclk */
> +	sensor->sensor_clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(sensor->sensor_clk))
> +		return dev_err_probe(dev, PTR_ERR(sensor->sensor_clk),
> +				     "Failed to get xvclk\n");
> +
> +	retval = ox05b1s_get_regulators(sensor);
> +	if (retval)
> +		return dev_err_probe(dev, retval, "Failed to get regulators\n");
> +
> +	sd = &sensor->subdev;
> +	v4l2_i2c_subdev_init(sd, client, &ox05b1s_subdev_ops);
> +	sd->internal_ops = &ox05b1s_internal_ops;
> +	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	sd->dev = &client->dev;
> +	sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +	sensor->pads[OX05B1S_SENS_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
> +	retval = media_entity_pads_init(&sd->entity, OX05B1S_SENS_PADS_NUM,
> +					sensor->pads);
> +	if (retval)
> +		goto probe_out;
> +
> +	mutex_init(&sensor->lock);

This could be devm_mutex_init().
This would slighlty simplify the remove function.
Otherwise, a mutex_destroy() is potentialy missing in the error handling 
of the probe.

> +
> +	retval = ox05b1s_init_controls(sensor);
> +	if (retval)
> +		goto probe_err_entity_cleanup;
> +
> +	/* power on manually */
> +	retval = ox05b1s_power_on(sensor);
> +	if (retval) {
> +		dev_err(dev, "Failed to power on\n");

dev_err_probe() can still be used to be consistent with other error path 
above.

> +		goto probe_err_free_ctrls;
> +	}
> +
> +	pm_runtime_set_active(dev);
> +	pm_runtime_get_noresume(dev);
> +	pm_runtime_enable(dev);
> +
> +	retval = ox05b1s_read_chip_id(sensor);
> +	if (retval)
> +		goto probe_err_pm_runtime;
> +
> +	v4l2_i2c_subdev_set_name(sd, client, sensor->model->name, NULL);
> +
> +	/* Centrally managed subdev active state */
> +	sd->state_lock = &sensor->lock;
> +	retval = v4l2_subdev_init_finalize(sd);
> +	if (retval < 0) {
> +		dev_err(dev, "Subdev init error: %d\n", retval);

Same

> +		goto probe_err_pm_runtime;
> +	}
> +
> +	retval = v4l2_async_register_subdev_sensor(sd);
> +	if (retval < 0) {
> +		dev_err(&client->dev, "Async register failed, ret=%d\n", retval);

Same

> +		goto probe_err_subdev_cleanup;
> +	}
> +
> +	sensor->mode = &sensor->model->supported_modes[0];
> +	ox05b1s_update_controls(sensor);
> +
> +	pm_runtime_set_autosuspend_delay(dev, 1000);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return 0;
> +
> +probe_err_subdev_cleanup:
> +	v4l2_subdev_cleanup(sd);
> +probe_err_pm_runtime:
> +	pm_runtime_put_noidle(dev);
> +	pm_runtime_disable(dev);
> +	ox05b1s_runtime_suspend(dev);
> +probe_err_free_ctrls:
> +	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
> +probe_err_entity_cleanup:
> +	media_entity_cleanup(&sd->entity);
> +probe_out:
> +	return retval;
> +}

...

> +static struct i2c_driver ox05b1s_i2c_driver = {
> +	.driver = {
> +		.name  = "ox05b1s",

2 spaces after .name.

> +		.pm = pm_ptr(&ox05b1s_pm_ops),
> +		.of_match_table	= ox05b1s_of_match,
> +	},
> +	.probe	= ox05b1s_probe,
> +	.remove = ox05b1s_remove,
> +};
> +
> +module_i2c_driver(ox05b1s_i2c_driver);
> +MODULE_DESCRIPTION("Omnivision OX05B1S MIPI Camera Subdev Driver");
> +MODULE_AUTHOR("Mirela Rabulea <mirela.rabulea@....com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/media/i2c/ox05b1s/ox05b1s_modes.c b/drivers/media/i2c/ox05b1s/ox05b1s_modes.c
> new file mode 100644
> index 000000000000..1f3b822d4482
> --- /dev/null
> +++ b/drivers/media/i2c/ox05b1s/ox05b1s_modes.c
> @@ -0,0 +1,63 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Register configurations for all sensor supported modes
> + * Copyright (C) 2024, NXP
> + * Copyright (C) 2024, Omnivision
> + * Copyright (C) 2024, Verisilicon
> + *
> + */
> +
> +#include "ox05b1s.h"
> +
> +/*
> + * Register configuration for Omnivision OX05B1S raw camera
> + * 2592X1944_30FPS_FULL_RGBIr 2592 1944
> + */
> +struct ox05b1s_reg ovx5b_init_setting_2592x1944[] = {

I think this could easily be made a const struct.

> +	{ 0x0107, 0x01 },
> +	{ 0x0307, 0x02 },
> +	{ 0x034a, 0x05 },
> +	{ 0x040b, 0x5c },
> +	{ 0x040c, 0xcd },
> +	{ 0x3009, 0x2e },
> +	{ 0x3219, 0x08 },
> +	{ 0x3684, 0x6d },
> +	{ 0x3685, 0x6d },
> +	{ 0x3686, 0x6d },
> +	{ 0x3687, 0x6d },
> +	{ 0x368c, 0x07 },
> +	{ 0x368d, 0x07 },
> +	{ 0x368e, 0x07 },
> +	{ 0x368f, 0x00 },
> +	{ 0x3690, 0x04 },
> +	{ 0x3691, 0x04 },
> +	{ 0x3692, 0x04 },
> +	{ 0x3693, 0x04 },
> +	{ 0x3698, 0x00 },
> +	{ 0x36a0, 0x05 },
> +	{ 0x36a2, 0x16 },
> +	{ 0x36a3, 0x03 },
> +	{ 0x36a4, 0x07 },
> +	{ 0x36a5, 0x24 },
> +	{ 0x36e3, 0x09 },
> +	{ 0x3702, 0x0a },
> +	{ 0x3821, 0x04 }, /* mirror */
> +	{ 0x3822, 0x10 },
> +	{ 0x382b, 0x03 },
> +	{ 0x3866, 0x10 },
> +	{ 0x386c, 0x46 },
> +	{ 0x386d, 0x08 },
> +	{ 0x386e, 0x7b },
> +	{ 0x4802, 0x00 },
> +	{ 0x481b, 0x3c },
> +	{ 0x4837, 0x19 },
> +	{ /* sentinel*/ },

Nitpick: no need for ending comma after a terminator.

> +};
> +
> +struct ox05b1s_reglist ox05b1s_reglist_2592x1944[] = {

I think this could easily be made a const struct.

> +	{
> +		.regs = ovx5b_init_setting_2592x1944
> +	}, {
> +		/* sentinel */
> +	}
> +};

Powered by blists - more mailing lists