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

From: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
To: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	Maxime Ripard <mripard@kernel.org>,
	Hans Verkuil <hverkuil@xs4all.nl>,
	Rob Herring <robh+dt@kernel.org>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	linux-arm-kernel@lists.infradead.org,
	linux-media@vger.kernel.org
Subject: Re: [PATCH v4 2/2] media: i2c: Add support for the OV5648 image sensor
Date: Mon, 30 Nov 2020 14:48:04 +0100	[thread overview]
Message-ID: <X8T4FH/QjRdLu0hS@aptenodytes> (raw)
In-Reply-To: <20201130092802.GU3940@paasikivi.fi.intel.com>

[-- Attachment #1.1: Type: text/plain, Size: 1594 bytes --]

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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel