Re: [PATCH v2 11/11] mt9m111: make use of testpattern

2010-09-05 Thread Robert Jarzmik
Guennadi Liakhovetski g.liakhovet...@gmx.de writes:

 Yes, but this has another disadvantage - if you do not use s_register / 
 g_register, maybe you just have CONFIG_VIDEO_ADV_DEBUG off, then, once you 
 load the module with the testpattern parameter, you cannot switch using 
 testpatterns off again (without a reboot or a power cycle). With the 
 original version you can load the driver with the parameter set, then 
 unload it, load it without the parameter and testpattern would be cleared. 
 In general, I think, using direct register access is discouraged, 
 especially if there's a way to set the same functionality using driver's 
 supported interfaces.

I agree. If there is a way without debug registers, let's use it.

 Hm, if I'm not mistaken, it has once been mentioned, that these test-patterns
 can be nicely implemented using the S_INPUT ioctl(). Am I right? How about
 that? But we'd need a confirmation for that, I'm not 100% sure.
I can't remember that. But if there is a standard ioctl (as seems to show
videodev2.h), and that its use could mean camera's input is a testpattern or
camera input is the normal optical flow, then we should use it.
If not, the old way with debug registers is the only alternative I see without
having to unload/reload the module (if it's a module and not statically embedded
in the kernel).

Cheers.

--
Robert
--
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 v2 11/11] mt9m111: make use of testpattern

2010-08-29 Thread Robert Jarzmik
Michael Grzeschik m.grzesc...@pengutronix.de writes:

 Signed-off-by: Philipp Wiesner p.wies...@phytec.de
 Signed-off-by: Michael Grzeschik m.grzesc...@pengutronix.de

I would require a small change here.

I am using the testpattern for non regression tests. This change implies that
the test pattern can only be set up by module parameters, and blocks the usage
through V4L2 debug, registers, see below:
memset(set_reg, 0, sizeof(set_reg));
set_reg.match.type = V4L2_CHIP_MATCH_I2C_ADDR;
set_reg.match.addr = 0x5d;
set_reg.reg = 0x148;
set_reg.val = test_pattern;
set_reg.size = 1;
if (test_pattern != -1)
if (-1 == xioctl (fd, VIDIOC_DBG_S_REGISTER, set_reg)) {
fprintf (stderr, %s could set test pattern %x\n,
 dev_name, test_pattern);
exit (EXIT_FAILURE);
}

But, the idea is not bad. Therefore, I'd like you to change:
 + dev_dbg(client-dev, %s: using testpattern %d\n, __func__,
 + testpattern);
 +
 + if (!ret)
 + ret = mt9m111_reg_set(client,
 + MT9M111_TEST_PATTERN_GEN, pattern);
into
 + dev_dbg(client-dev, %s: using testpattern %d\n, __func__,
 + testpattern);
 +
 + if (!ret  pattern)
 + ret = mt9m111_reg_set(client,
 + MT9M111_TEST_PATTERN_GEN, pattern);
 +

This way, the V4L2 debug registers usage is still allowed, and your module
parameter works too.

Cheers.

--
Robert
--
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 v2 11/11] mt9m111: make use of testpattern

2010-08-29 Thread Guennadi Liakhovetski
On Sun, 29 Aug 2010, Robert Jarzmik wrote:

 Michael Grzeschik m.grzesc...@pengutronix.de writes:
 
  Signed-off-by: Philipp Wiesner p.wies...@phytec.de
  Signed-off-by: Michael Grzeschik m.grzesc...@pengutronix.de
 
 I would require a small change here.
 
 I am using the testpattern for non regression tests. This change implies that
 the test pattern can only be set up by module parameters, and blocks the usage
 through V4L2 debug, registers, see below:
 memset(set_reg, 0, sizeof(set_reg));
 set_reg.match.type = V4L2_CHIP_MATCH_I2C_ADDR;
 set_reg.match.addr = 0x5d;
 set_reg.reg = 0x148;
 set_reg.val = test_pattern;
 set_reg.size = 1;
 if (test_pattern != -1)
 if (-1 == xioctl (fd, VIDIOC_DBG_S_REGISTER, set_reg)) {
 fprintf (stderr, %s could set test pattern %x\n,
  dev_name, test_pattern);
 exit (EXIT_FAILURE);
 }
 
 But, the idea is not bad. Therefore, I'd like you to change:
  +   dev_dbg(client-dev, %s: using testpattern %d\n, __func__,
  +   testpattern);
  +
  +   if (!ret)
  +   ret = mt9m111_reg_set(client,
  +   MT9M111_TEST_PATTERN_GEN, pattern);
 into
  +   dev_dbg(client-dev, %s: using testpattern %d\n, __func__,
  +   testpattern);
  +
  +   if (!ret  pattern)
  +   ret = mt9m111_reg_set(client,
  +   MT9M111_TEST_PATTERN_GEN, pattern);
  +
 
 This way, the V4L2 debug registers usage is still allowed, and your module
 parameter works too.

Yes, but this has another disadvantage - if you do not use s_register / 
g_register, maybe you just have CONFIG_VIDEO_ADV_DEBUG off, then, once you 
load the module with the testpattern parameter, you cannot switch using 
testpatterns off again (without a reboot or a power cycle). With the 
original version you can load the driver with the parameter set, then 
unload it, load it without the parameter and testpattern would be cleared. 
In general, I think, using direct register access is discouraged, 
especially if there's a way to set the same functionality using driver's 
supported interfaces. Hm, if I'm not mistaken, it has once been mentioned, 
that these test-patterns can be nicely implemented using the S_INPUT 
ioctl(). Am I right? How about that? But we'd need a confirmation for 
that, I'm not 100% sure.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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