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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3g7utet7n4gkhuc4wmq23n45u35n2nmuoyizled5lb3ra23ar4@nuf2ekb7houm>
Date: Wed, 27 Nov 2024 09:31:53 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Mirela Rabulea <mirela.rabulea@....com>
Cc: 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, 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 v2 2/4] media: ox05b1s: Add omnivision OX05B1S raw sensor
 driver

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