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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Fri, Jan 24, 2025 at 09:07:01AM +0100, Krzysztof Kozlowski wrote:
> On Fri, Jan 24, 2025 at 02:12:41AM +0200, Mirela Rabulea wrote:
> > +
> > +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;
> > +
> > +	ret = cci_read(sensor->regmap, OX05B1S_REG_CHIP_ID, &chip_id, NULL);
> 
> This suggests these are compatible devices. But in the binding you said
> they are not... so your binding is not correct. Express compatibility
> with proper fallback.

Unfortunately we can't do that for camera sensors. It's important for
drivers to be able to identify the camera sensor model without having to
power up the device at probe time, depending on what device the sensor
is integrated in. For instance, with camera sensors found in laptops,
the privacy LED is often connected to the sensor power supply, and a
flashing privacy light when booting scares users.

Reading the ID register at probe time is fine when running on a platform
where the sensor can be powered up when probing, so the driver isn't
wrong doing so. It's also fine for drivers to not implement the no-power
probe right away. DT bindings however need to be ready for this, so a
fallback string can't be used.

> > +	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;
> > +	}
> > +
> > +	return ret;
> > +}
> 
> ...
> 
> > +
> > +static const struct of_device_id ox05b1s_of_match[] = {
> > +	{
> > +		.compatible = "ovti,ox05b1s",
> 
> And this is incomplete, according to current binding, so it seems you
> wanted to make them compatible just did not write the binding like that?
> 
> Confusing.
> 
> > +		.data = &ox05b1s_data,
> > +	},
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, ox05b1s_of_match);
> > +
> > +static struct i2c_driver ox05b1s_i2c_driver = {
> > +	.driver = {
> > +		.name  = "ox05b1s",
> > +		.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@nxp.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[] = {
> 
> Why this is not const? I commented last time with the same.
> 
> > +	{ 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*/ },
> > +};
> > +
> > +struct ox05b1s_reglist ox05b1s_reglist_2592x1944[] = {
> 
> Why not const?
> 
> Best regards,
> Krzysztof
> 

-- 
Regards,

Laurent Pinchart