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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux