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