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
