* [PATCH] media: cxd2099: move pre-init values out of init() @ 2015-02-08 20:55 luisbg 2015-04-08 11:09 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 3+ messages in thread From: luisbg @ 2015-02-08 20:55 UTC (permalink / raw) To: linux-media; +Cc: devel, linux-kernel, mchehab, gregkh Improve code readability by moving out all pre-init values from the init function. Signed-off-by: Luis de Bethencourt <luis.bg@samsung.com> --- drivers/staging/media/cxd2099/cxd2099.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/staging/media/cxd2099/cxd2099.c b/drivers/staging/media/cxd2099/cxd2099.c index 657ea48..bafe36f 100644 --- a/drivers/staging/media/cxd2099/cxd2099.c +++ b/drivers/staging/media/cxd2099/cxd2099.c @@ -300,7 +300,6 @@ static int init(struct cxd *ci) int status; mutex_lock(&ci->lock); - ci->mode = -1; do { status = write_reg(ci, 0x00, 0x00); if (status < 0) @@ -420,7 +419,6 @@ static int init(struct cxd *ci) status = write_regm(ci, 0x09, 0x08, 0x08); if (status < 0) break; - ci->cammode = -1; cam_mode(ci, 0); } while (0); mutex_unlock(&ci->lock); @@ -711,6 +709,8 @@ struct dvb_ca_en50221 *cxd2099_attach(struct cxd2099_cfg *cfg, ci->en = en_templ; ci->en.data = ci; + ci->mode = -1; + ci->cammode = -1; init(ci); dev_info(&i2c->dev, "Attached CXD2099AR at %02x\n", ci->cfg.adr); return &ci->en; -- 2.1.0 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] media: cxd2099: move pre-init values out of init() 2015-02-08 20:55 [PATCH] media: cxd2099: move pre-init values out of init() luisbg @ 2015-04-08 11:09 ` Mauro Carvalho Chehab 2015-04-08 12:35 ` Luis de Bethencourt 0 siblings, 1 reply; 3+ messages in thread From: Mauro Carvalho Chehab @ 2015-04-08 11:09 UTC (permalink / raw) To: luisbg; +Cc: linux-media, devel, linux-kernel, gregkh Em Sun, 8 Feb 2015 20:55:36 +0000 luisbg <luis@debethencourt.com> escreveu: > Improve code readability by moving out all pre-init values from the init > function. > > Signed-off-by: Luis de Bethencourt <luis.bg@samsung.com> > --- > drivers/staging/media/cxd2099/cxd2099.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/media/cxd2099/cxd2099.c b/drivers/staging/media/cxd2099/cxd2099.c > index 657ea48..bafe36f 100644 > --- a/drivers/staging/media/cxd2099/cxd2099.c > +++ b/drivers/staging/media/cxd2099/cxd2099.c > @@ -300,7 +300,6 @@ static int init(struct cxd *ci) > int status; > > mutex_lock(&ci->lock); > - ci->mode = -1; > do { > status = write_reg(ci, 0x00, 0x00); > if (status < 0) > @@ -420,7 +419,6 @@ static int init(struct cxd *ci) > status = write_regm(ci, 0x09, 0x08, 0x08); > if (status < 0) > break; > - ci->cammode = -1; > cam_mode(ci, 0); > } while (0); > mutex_unlock(&ci->lock); > @@ -711,6 +709,8 @@ struct dvb_ca_en50221 *cxd2099_attach(struct cxd2099_cfg *cfg, > > ci->en = en_templ; > ci->en.data = ci; > + ci->mode = -1; > + ci->cammode = -1; This actually changes the logic, as, cammode is == -1 only if the do {} while loop succeeds. Also, calling cam_mode(ci, 0) will change cammode to 0. Btw, for it to work, ci->mode should be initialized earlier. So, this patch looks very wrong on my eyes, except if you found a real bug on it. Have you tested it on a real device? What bug does it fix? Regards, Mauro > init(ci); > dev_info(&i2c->dev, "Attached CXD2099AR at %02x\n", ci->cfg.adr); > return &ci->en; ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] media: cxd2099: move pre-init values out of init() 2015-04-08 11:09 ` Mauro Carvalho Chehab @ 2015-04-08 12:35 ` Luis de Bethencourt 0 siblings, 0 replies; 3+ messages in thread From: Luis de Bethencourt @ 2015-04-08 12:35 UTC (permalink / raw) To: Mauro Carvalho Chehab; +Cc: linux-media, devel, linux-kernel, gregkh On Wed, Apr 08, 2015 at 08:09:03AM -0300, Mauro Carvalho Chehab wrote: > Em Sun, 8 Feb 2015 20:55:36 +0000 > luisbg <luis@debethencourt.com> escreveu: > > > Improve code readability by moving out all pre-init values from the init > > function. > > > > Signed-off-by: Luis de Bethencourt <luis.bg@samsung.com> > > --- > > drivers/staging/media/cxd2099/cxd2099.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/staging/media/cxd2099/cxd2099.c b/drivers/staging/media/cxd2099/cxd2099.c > > index 657ea48..bafe36f 100644 > > --- a/drivers/staging/media/cxd2099/cxd2099.c > > +++ b/drivers/staging/media/cxd2099/cxd2099.c > > @@ -300,7 +300,6 @@ static int init(struct cxd *ci) > > int status; > > > > mutex_lock(&ci->lock); > > - ci->mode = -1; > > do { > > status = write_reg(ci, 0x00, 0x00); > > if (status < 0) > > @@ -420,7 +419,6 @@ static int init(struct cxd *ci) > > status = write_regm(ci, 0x09, 0x08, 0x08); > > if (status < 0) > > break; > > - ci->cammode = -1; > > cam_mode(ci, 0); > > } while (0); > > mutex_unlock(&ci->lock); > > @@ -711,6 +709,8 @@ struct dvb_ca_en50221 *cxd2099_attach(struct cxd2099_cfg *cfg, > > > > ci->en = en_templ; > > ci->en.data = ci; > > + ci->mode = -1; > > + ci->cammode = -1; > > This actually changes the logic, as, cammode is == -1 only if the > do {} while loop succeeds. > > Also, calling cam_mode(ci, 0) will change cammode to 0. Btw, for > it to work, ci->mode should be initialized earlier. > > So, this patch looks very wrong on my eyes, except if you found > a real bug on it. > > Have you tested it on a real device? What bug does it fix? > > Regards, > Mauro > Apologies. You are right and this patch is completely wrong. I submitted this while reading the code of a few drivers to learn from them. I should've retracted it before you spent time reviewing it. Sorry for that. Thanks, Luis > > init(ci); > > dev_info(&i2c->dev, "Attached CXD2099AR at %02x\n", ci->cfg.adr); > > return &ci->en; ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-04-08 12:39 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-02-08 20:55 [PATCH] media: cxd2099: move pre-init values out of init() luisbg 2015-04-08 11:09 ` Mauro Carvalho Chehab 2015-04-08 12:35 ` Luis de Bethencourt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox