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

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

 



2018-04-09 16:36 GMT+09:00 jacopo mondi <jacopo@xxxxxxxxxx>:
> 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@xxxxxxxxxx>
>> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>> Cc: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>> Cc: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
>> Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxxx>
>> Signed-off-by: Akinobu Mita <akinobu.mita@xxxxxxxxx>
>> ---
>>  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.

OK. This needs to change the data types of pid and ver from 'u8' to 'int'.

>>
>>       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?

Sounds good.

> 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
>>



[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