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

[Paul Kocialkowski]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20201130/d5cbeb40/attachment.sig>