| Message ID | 1523116090-13101-3-git-send-email-akinobu.mita@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
2018-04-09 16:36 GMT+09:00 jacopo mondi <jacopo@jmondi.org>: > 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. 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 >>
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; switch (VERSION(pid, ver)) { case OV7720:
This change adds checks for register read errors and returns correct 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(-)