Re: [PATCH 2/6] media: ov772x: add checks for register read errors

[jacopo mondi <jacopo@jmondi.org>]

[-- Attachment #1: Type: text/plain, Size: 1879 bytes --]

Hi Akinobu,

On Sun, Apr 08, 2018 at 12:48:06AM +0900, Akinobu Mita wrote:
> This change adds checks for register read errors and returns correct
> error code.
>

I feel like error conditions are anyway captured by the switch()
default case, but I understand there may be merits in returning the
actual error code.

> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  drivers/media/i2c/ov772x.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 283ae2c..c56f910 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -1169,8 +1169,15 @@ static int ov772x_video_probe(struct ov772x_priv *priv)
>  		return ret;
>
>  	/* Check and show product ID and manufacturer ID. */
> -	pid = ov772x_read(client, PID);
> -	ver = ov772x_read(client, VER);
> +	ret = ov772x_read(client, PID);
> +	if (ret < 0)
> +		return ret;
> +	pid = ret;
> +
> +	ret = ov772x_read(client, VER);
> +	if (ret < 0)
> +		return ret;
> +	ver = ret;

You can assign the ov772x_read() return value to pid and ver directly
and save two assignments.

>
>  	switch (VERSION(pid, ver)) {
>  	case OV7720:

If we want to check for return values here, which is always a good
thing, could you do the same for MIDH and MIDL below?

Nit: You can also fix the dev_info() parameters alignment to span to
the whole line length while at there. Ie.

	dev_info(&client->dev,
		 "%s Product ID %0x:%0x Manufacturer ID %x:%x\n",
		 devname, pid, ver, ov772x_read(client, MIDH),
		 ov772x_read(client, MIDL));

Thanks
   j


> --
> 2.7.4
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]