linux-kernel - Re: [PATCH v4 2/2] media: i2c: Add support for the OV5648 image sensor

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: <X8T4FH/QjRdLu0hS@aptenodytes>
Date:   Mon, 30 Nov 2020 14:48:04 +0100
From:   Paul Kocialkowski <paul.kocialkowski@...tlin.com>
To:     Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc:     linux-media@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Maxime Ripard <mripard@...nel.org>,
        Hans Verkuil <hverkuil@...all.nl>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v4 2/2] media: i2c: Add support for the OV5648 image
 sensor

Hi Sakari,

On Mon 30 Nov 20, 11:28, Sakari Ailus wrote:
> Hi Paul,
> 
> Thanks for the update. I have a few comments on the driver, too.

Thanks for the review!

[...]

> > +	ret = ov5648_write(sensor, OV5648_GAIN_BLUE_MAN_L_REG,
> > +			   OV5648_GAIN_BLUE_MAN_L(blue_balance));
> 
> return ...
> 
> Same below (and above).

Well I don't think that makes any functional difference, right?
My personal preference is to have explicit checks even at the end of functions
for symetry and alignment with other blocks.

If it's okay, it'd like to keep it as-is. But if that's against kernel coding
style guidelines, I won't argue more.

[...]

> > +	/* Gain */
> > +
> > +	ctrls->gain_auto =
> > +		v4l2_ctrl_new_std(handler, ops, V4L2_CID_AUTOGAIN, 0, 1, 1, 1);
> > +
> > +	ctrls->gain = v4l2_ctrl_new_std(handler, ops, V4L2_CID_GAIN, 16, 1023,
> > +					16, 16);
> > +	ctrls->gain->flags |= V4L2_CTRL_FLAG_VOLATILE;
> 
> Note that ctrls->gain may be NULL here. Please move after the handler's
> error check. Same for link_freq and pixel_rate.

You're right, this is not very safe, I'll fix it.

[...]

> > +static int ov5648_remove(struct i2c_client *client)
> > +{
> > +	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> > +	struct ov5648_sensor *sensor = ov5648_subdev_sensor(subdev);
> > +
> > +	clk_rate_exclusive_put(sensor->xvclk);
> 
> This seems to be extra now.

Good catch, that's indeed a mistake!

Cheers and thanks,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)