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

[Krzysztof Kozlowski <krzk@xxxxxxxxxx>]

On Tue, Nov 26, 2024 at 05:50:58PM +0200, Mirela Rabulea wrote:
> +struct ox05b1s {
> +	struct i2c_client *i2c_client;
> +	struct regmap *regmap;
> +	struct gpio_desc *rst_gpio;
> +	struct regulator_bulk_data supplies[OX05B1S_NUM_SUPPLIES];
> +	struct clk *sensor_clk;
> +	const struct ox05b1s_plat_data *model;
> +	struct v4l2_subdev subdev;
> +	struct media_pad pads[OX05B1S_SENS_PADS_NUM];
> +	const struct ox05b1s_mode *mode;
> +	struct mutex lock; /* sensor lock */
> +	u32 stream_status;
> +	struct ox05b1s_ctrls ctrls;
> +};
> +
> +static struct ox05b1s_mode ox05b1s_supported_modes[] = {

This is const, I think.

> +	{
> +		.index		= 0,
> +		.width		= 2592,
> +		.height		= 1944,
> +		.code		= MEDIA_BUS_FMT_SGRBG10_1X10,
> +		.bpp		= 10,
> +		.vts		= 0x850,
> +		.hts		= 0x2f0,
> +		.exp		= 0x850 - 8,
> +		.h_bin		= false,
> +		.fps		= 30,
> +		.reg_data	= ox05b1s_reglist_2592x1944,
> +	}, {
> +		/* sentinel */
> +	}
> +};

...

> +	ret = ox05b1s_read_reg(sensor, OX05B1S_REG_CHIP_ID_23_16, &reg_val);
> +	chip_id |= reg_val << 16;
> +	ret |= ox05b1s_read_reg(sensor, OX05B1S_REG_CHIP_ID_15_8, &reg_val);
> +	chip_id |= reg_val << 8;
> +	ret |= ox05b1s_read_reg(sensor, OX05B1S_REG_CHIP_ID_7_0, &reg_val);
> +	chip_id |= reg_val;
> +	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_info(dev, "Camera %s detected, chip_id=%x\n", camera_name, chip_id);

Device should be silent on success. This can be dev_dbg.

> +	} else {
> +		dev_err(dev, "Detected %s camera (chip_id=%x), but expected %s (chip_id=%x)\n",
> +			camera_name, chip_id, sensor->model->name, sensor->model->chip_id);
> +		ret = -ENODEV;
> +	}
> +
> +	return ret;
> +}
> +
> +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))
> +		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)) {

No need for {}, some left-over from old version.

Best regards,
Krzysztof