Re: [PATCH] media: cxd2099: move pre-init values out of init()

2015-04-08 Thread Mauro Carvalho Chehab 
Em Sun, 8 Feb 2015 20:55:36 +
luisbg l...@debethencourt.com escreveu:

 Improve code readability by moving out all pre-init values from the init
 function.
 
 Signed-off-by: Luis de Bethencourt luis...@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;
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] media: cxd2099: move pre-init values out of init()

2015-04-08 Thread Luis de Bethencourt 
On Wed, Apr 08, 2015 at 08:09:03AM -0300, Mauro Carvalho Chehab wrote:
 Em Sun, 8 Feb 2015 20:55:36 +
 luisbg l...@debethencourt.com escreveu:
 
  Improve code readability by moving out all pre-init values from the init
  function.
  
  Signed-off-by: Luis de Bethencourt luis...@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;
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] media: cxd2099: move pre-init values out of init()

2015-02-08 Thread luisbg 
Improve code readability by moving out all pre-init values from the init
function.

Signed-off-by: Luis de Bethencourt luis...@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

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html