[v2,11/11] mt9m111: make use of testpattern

  Signed-off-by: Philipp Wiesner <p.wiesner@phytec.de>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
Changes v1 -> v2
	* removed ifdef DEBUG

 drivers/media/video/mt9m111.c |   57 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 57 insertions(+), 0 deletions(-)
  

Michael Grzeschik <m.grzeschik@pengutronix.de> writes:

> Signed-off-by: Philipp Wiesner <p.wiesner@phytec.de>
> Signed-off-by: Michael Grzeschik <m.grzeschik@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Sun, 29 Aug 2010, Robert Jarzmik wrote:

> Michael Grzeschik <m.grzeschik@pengutronix.de> writes:
> 
> > Signed-off-by: Philipp Wiesner <p.wiesner@phytec.de>
> > Signed-off-by: Michael Grzeschik <m.grzeschik@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Guennadi Liakhovetski <g.liakhovetski@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html