em28xx: enable usb audio for plextor px-tv100u

PATCH In review [email protected] 3 participants 13 messages Jul 18, 2009 – Jul 30, 2009 Media (V4L/DVB)

lore

Share & tools

lore

[email protected]

author

2009-07-18 17:38 UTC #0 lore raw

PATCH 1/1 em28xx: enable usb audio for plextor px-tv100u

linux/drivers/media/video/em28xx/em28xx-cards.c +5 -1

diff -r 27ddf3fe0ed9 linux/drivers/media/video/em28xx/em28xx-cards.c
@@ -639,10 +639,10 @@ struct em28xx_board em28xx_boards[] = {
 	},
 	[EM2861_BOARD_PLEXTOR_PX_TV100U] = {
 		.name         = "Plextor ConvertX PX-TV100U",
-		.valid        = EM28XX_BOARD_NOT_VALIDATED,
 		.tuner_type   = TUNER_TNF_5335MF,
 		.tda9887_conf = TDA9887_PRESENT,
 		.decoder      = EM28XX_TVP5150,
+		.has_msp34xx  = 1,
 		.input        = { {
 			.type     = EM28XX_VMUX_TELEVISION,
 			.vmux     = TVP5150_COMPOSITE0,

@@ -1950,6 +1950,10 @@ void em28xx_pre_card_setup(struct em28xx
 		/* FIXME guess */
 		/* Turn on analog audio output */
 		em28xx_write_reg(dev, EM28XX_R08_GPIO, 0xfd);
+
+		/* enable audio 12mhz i2s */
+		em28xx_write_reg(dev, EM28XX_R0F_XCLK, 0xa7);
+		dev->i2s_speed = 2048000;
 		break;
 	case EM2861_BOARD_KWORLD_PVRTV_300U:
 	case EM2880_BOARD_KWORLD_DVB_305U:

linux/drivers/media/video/em28xx/em28xx-video.c +5 -2

diff -r 27ddf3fe0ed9 linux/drivers/media/video/em28xx/em28xx-video.c
@@ -1087,9 +1087,12 @@ static int vidioc_s_ctrl(struct file *fi
 
 	mutex_lock(&dev->lock);
 
-	if (dev->board.has_msp34xx)
+	if (dev->board.has_msp34xx) {
+		/*FIXME hack to unmute usb audio stream */
+		em28xx_set_ctrl(dev, ctrl);
+
 		v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_ctrl, ctrl);
-	else {
+	} else {
 		rc = 1;
 		for (i = 0; i < ARRAY_SIZE(em28xx_qctrl); i++) {
 			if (ctrl->id == em28xx_qctrl[i].id) {

Mauro Carvalho Chehab

2009-07-29 04:57 UTC #3 lore raw

Em Tue, 28 Jul 2009 20:07:53 -0400
[email protected] escreveu:

Quoted 6 lines · Mauro Carvalho Chehab

Quoted 1 line
Hi Acano,
Tested-by Angelo Cano <[email protected]>
works great

Good!

Quoted 15 lines

Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+		/*FIXME hack to unmute usb audio stream */
+		em28xx_set_ctrl(dev, ctrl);
Hmm... this function were removed. In thesis, you shouldn't need to do anything to unmute.
I still need it, see attachment.
Quoted 5 lines
Could you please try the enclosed patch and see if this is enough to fix for Plextor? If so, please send me a Tested-by: tag for me to add it at 2.6.31 fix patches.

Quoted 5 lines

Like I said the patch works great, and also solves my audio volume problem. With your patch the volume is set to a sane value (presumably 0db) and the distortion/clipping is gone.
Thanks man. The volume problem was driving me crazy.

Ah, yes, there's a missing mute/unmute issue there. Instead of using your code, I opted to duplicate part of ac97_set_ctrl code there.

I opted to have a small duplicated code, but, IMO, it is now clearer to see why we still need to call em28xx_audio_analog_set(). You will notice that I've rearranged the place where I update volume and mute. The rationale is that v4l2_device_call_all() might eventually change a value for volume/mute.

Another reason is that, IMO, v4l2_device_call_all() should return values. In the specific case of volume/mute, if the user tries to specify a value outside the range, the -ERANGE should be returned.

I've already committed the patches at the tree. Please double-check.

Hans,

we need to fix the returned error value for v4l2_device_call_all(). I know that this is an old issue that weren't changed by v4l dev/subdev conversion, but now it is easier for us to fix. The idea here is to be sure that, if a sub-driver with a proper handling for a function returns an error value, this would be returned by v4l2_device_call_all(). Maybe we'll need to adjust some things at the sub-drivers.

Cheers,
Mauro

Hans Verkuil

2009-07-29 06:09 UTC #4 lore raw

Quoted 45 lines · On Wednesday 29 July 2009 06:57:30 Mauro Carvalho Chehab wrote:

Em Tue, 28 Jul 2009 20:07:53 -0400
[email protected] escreveu:
Quoted 6 lines · Mauro Carvalho Chehab
Quoted 1 line
Hi Acano,
Tested-by Angelo Cano <[email protected]>
works great
Good!
Quoted 12 lines
Quoted 5 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+		/*FIXME hack to unmute usb audio stream */
+		em28xx_set_ctrl(dev, ctrl);
Hmm... this function were removed. In thesis, you shouldn't need to do anything to unmute.
I still need it, see attachment.
Quoted 3 lines
Could you please try the enclosed patch and see if this is enough to fix for Plextor? If so, please send me a Tested-by: tag for me to add it at 2.6.31 fix patches.
Quoted 5 lines
Like I said the patch works great, and also solves my audio volume problem. With your patch the volume is set to a sane value (presumably 0db) and the distortion/clipping is gone.
Thanks man. The volume problem was driving me crazy.
Ah, yes, there's a missing mute/unmute issue there. Instead of using your code, I opted to duplicate part of ac97_set_ctrl code there.
I opted to have a small duplicated code, but, IMO, it is now clearer to see why we still need to call em28xx_audio_analog_set(). You will notice that I've rearranged the place where I update volume and mute. The rationale is that v4l2_device_call_all() might eventually change a value for volume/mute.
Another reason is that, IMO, v4l2_device_call_all() should return values. In the specific case of volume/mute, if the user tries to specify a value outside the range, the -ERANGE should be returned.
I've already committed the patches at the tree. Please double-check.

Quoted 8 lines

Hans,
we need to fix the returned error value for v4l2_device_call_all(). I know that this is an old issue that weren't changed by v4l dev/subdev conversion, but now it is easier for us to fix. The idea here is to be sure that, if a sub-driver with a proper handling for a function returns an error value, this would be returned by v4l2_device_call_all(). Maybe we'll need to adjust some things at the sub-drivers.

Use v4l2_device_call_until_err instead of v4l2_device_call_all. That macro checks for errors returned from the subdevs.

Regards,

Hans

Signature · 1 line

Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom

Mauro Carvalho Chehab

2009-07-29 12:40 UTC #5 lore raw

Em Wed, 29 Jul 2009 08:09:31 +0200
Hans Verkuil <[email protected]> escreveu:

Quoted 55 lines · On Wednesday 29 July 2009 06:57:30 Mauro Carvalho Chehab wrote:

Quoted 45 lines
Em Tue, 28 Jul 2009 20:07:53 -0400
[email protected] escreveu:
Quoted 6 lines · Mauro Carvalho Chehab
Quoted 1 line
Hi Acano,
Tested-by Angelo Cano <[email protected]>
works great
Good!
Quoted 12 lines
Quoted 5 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+		/*FIXME hack to unmute usb audio stream */
+		em28xx_set_ctrl(dev, ctrl);
Hmm... this function were removed. In thesis, you shouldn't need to do anything to unmute.
I still need it, see attachment.
Quoted 3 lines
Could you please try the enclosed patch and see if this is enough to fix for Plextor? If so, please send me a Tested-by: tag for me to add it at 2.6.31 fix patches.
Quoted 5 lines
Like I said the patch works great, and also solves my audio volume problem. With your patch the volume is set to a sane value (presumably 0db) and the distortion/clipping is gone.
Thanks man. The volume problem was driving me crazy.
Ah, yes, there's a missing mute/unmute issue there. Instead of using your code, I opted to duplicate part of ac97_set_ctrl code there.
I opted to have a small duplicated code, but, IMO, it is now clearer to see why we still need to call em28xx_audio_analog_set(). You will notice that I've rearranged the place where I update volume and mute. The rationale is that v4l2_device_call_all() might eventually change a value for volume/mute.
Another reason is that, IMO, v4l2_device_call_all() should return values. In the specific case of volume/mute, if the user tries to specify a value outside the range, the -ERANGE should be returned.
I've already committed the patches at the tree. Please double-check.
Quoted 8 lines
Hans,
we need to fix the returned error value for v4l2_device_call_all(). I know that this is an old issue that weren't changed by v4l dev/subdev conversion, but now it is easier for us to fix. The idea here is to be sure that, if a sub-driver with a proper handling for a function returns an error value, this would be returned by v4l2_device_call_all(). Maybe we'll need to adjust some things at the sub-drivers.

Quoted 2 lines

Use v4l2_device_call_until_err instead of v4l2_device_call_all. That macro checks for errors returned from the subdevs.

It doesn't work as expected. If I use it for queryctl, for example, it returns an empty set of controls. If I use it for g_ctrl, it returns:

error 22 getting ctrl Brightness
error 22 getting ctrl Contrast
error 22 getting ctrl Saturation
error 22 getting ctrl Hue
error 22 getting ctrl Volume
error 22 getting ctrl Balance
error 22 getting ctrl Bass
error 22 getting ctrl Treble
error 22 getting ctrl Mute
error 22 getting ctrl Loudness

The issue here is we need something that discards errors for non-implemented controls.

As the sub-drivers are returning -EINVAL for non-implemented controls (and probably other stuff that aren't implemented there), the function will not work for some ioctls.

The proper fix seems to elect an error condition to be returned by driver when a function is not implemented, and such errors to be discarded by the macro.

It seems that the proper error code for such case is this one:

#define ENOSYS 38 /* Function not implemented */

Cheers,
Mauro

Hans Verkuil

2009-07-29 13:14 UTC #6 lore raw

Quoted 99 lines

Em Wed, 29 Jul 2009 08:09:31 +0200 Hans Verkuil <[email protected]> escreveu:
Quoted 62 lines · On Wednesday 29 July 2009 06:57:30 Mauro Carvalho Chehab wrote:
Quoted 4 lines
Em Tue, 28 Jul 2009 20:07:53 -0400
[email protected] escreveu:
Quoted 1 line
On Mon, Jul 27, 2009 at 09:28:11PM -0300, Mauro Carvalho Chehab
Quoted 12 lines · wrote:
Quoted 5 lines
Quoted 1 line
Hi Acano,
Tested-by Angelo Cano <[email protected]>
works great
Good!
Quoted 4 lines
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+		/*FIXME hack to unmute usb audio stream */
+		em28xx_set_ctrl(dev, ctrl);
Hmm... this function were removed. In thesis, you shouldn't need
to
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
do anything to unmute.
I still need it, see attachment.
Quoted 1 line
Could you please try the enclosed patch and see if this is enough
to
Quoted 1 line
Quoted 1 line
Quoted 1 line
fix for Plextor? If so, please send me a Tested-by: tag for me to
add
Quoted 9 lines
Quoted 7 lines
Quoted 1 line
it at 2.6.31 fix patches.
Like I said the patch works great, and also solves my audio volume problem. With your patch the volume is set to a sane value (presumably 0db) and the distortion/clipping is gone.
Thanks man. The volume problem was driving me crazy.
Ah, yes, there's a missing mute/unmute issue there. Instead of using
your
Quoted 3 lines
code, I opted to duplicate part of ac97_set_ctrl code there.
I opted to have a small duplicated code, but, IMO, it is now clearer
to
Quoted 1 line
see why we still need to call em28xx_audio_analog_set(). You will
notice
Quoted 2 lines
that I've rearranged the place where I update volume and mute. The rationale is that v4l2_device_call_all() might eventually change a
value
Quoted 3 lines
for volume/mute.
Another reason is that, IMO, v4l2_device_call_all() should return
values.
Quoted 1 line
In the specific case of volume/mute, if the user tries to specify a
value
Quoted 10 lines
outside the range, the -ERANGE should be returned.
I've already committed the patches at the tree. Please double-check.
Hans,
we need to fix the returned error value for v4l2_device_call_all(). I know that this is an old issue that weren't changed by v4l dev/subdev conversion, but now it is easier for us to fix. The idea here is to be sure that, if a sub-driver with a proper handling for a function
Quoted 8 lines
returns
Quoted 1 line
an error value, this would be returned by v4l2_device_call_all().
Maybe
Quoted 1 line
we'll need to adjust some things at the sub-drivers.
Use v4l2_device_call_until_err instead of v4l2_device_call_all. That macro
checks for errors returned from the subdevs.
It doesn't work as expected. If I use it for queryctl, for example, it returns
an empty set of controls. If I use it for g_ctrl, it returns:
error 22 getting ctrl Brightness error 22 getting ctrl Contrast error 22 getting ctrl Saturation error 22 getting ctrl Hue error 22 getting ctrl Volume error 22 getting ctrl Balance error 22 getting ctrl Bass error 22 getting ctrl Treble error 22 getting ctrl Mute error 22 getting ctrl Loudness
The issue here is we need something that discards errors for non-implemented
controls.
As the sub-drivers are returning -EINVAL for non-implemented controls (and probably other stuff that aren't implemented there), the function will not work
for some ioctls.

Quoted 8 lines

The proper fix seems to elect an error condition to be returned by driver when
a function is not implemented, and such errors to be discarded by the macro.
It seems that the proper error code for such case is this one:
#define ENOSYS 38 /* Function not implemented */

You are right, this macro doesn't work for these control functions. It it is possible to implement a define like ENOSYS, but I prefer to work on generic control processing code that is embedded in the v4l2 framework. It looks like I'll finally have time to work on that this weekend.

Currently the control handling code in our v4l drivers is, to be blunt, a pile of crap. And it is ideal to move this into the v4l2 framework since 90% of this is common code.

Regards,

Hans

Signature · 1 line

Hans Verkuil - video4linux developer - sponsored by TANDBERG

Mauro Carvalho Chehab

2009-07-29 14:42 UTC #7 lore raw

Em Wed, 29 Jul 2009 15:14:22 +0200
"Hans Verkuil" <[email protected]> escreveu:

Quoted 104 lines

Quoted 104 lines
Em Wed, 29 Jul 2009 08:09:31 +0200 Hans Verkuil <[email protected]> escreveu:
Quoted 62 lines · On Wednesday 29 July 2009 06:57:30 Mauro Carvalho Chehab wrote:
Quoted 4 lines
Em Tue, 28 Jul 2009 20:07:53 -0400
[email protected] escreveu:
Quoted 1 line
On Mon, Jul 27, 2009 at 09:28:11PM -0300, Mauro Carvalho Chehab
Quoted 12 lines · wrote:
Quoted 5 lines
Quoted 1 line
Hi Acano,
Tested-by Angelo Cano <[email protected]>
works great
Good!
Quoted 4 lines
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+		/*FIXME hack to unmute usb audio stream */
+		em28xx_set_ctrl(dev, ctrl);
Hmm... this function were removed. In thesis, you shouldn't need
to
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
do anything to unmute.
I still need it, see attachment.
Quoted 1 line
Could you please try the enclosed patch and see if this is enough
to
Quoted 1 line
Quoted 1 line
Quoted 1 line
fix for Plextor? If so, please send me a Tested-by: tag for me to
add
Quoted 9 lines
Quoted 7 lines
Quoted 1 line
it at 2.6.31 fix patches.
Like I said the patch works great, and also solves my audio volume problem. With your patch the volume is set to a sane value (presumably 0db) and the distortion/clipping is gone.
Thanks man. The volume problem was driving me crazy.
Ah, yes, there's a missing mute/unmute issue there. Instead of using
your
Quoted 3 lines
code, I opted to duplicate part of ac97_set_ctrl code there.
I opted to have a small duplicated code, but, IMO, it is now clearer
to
Quoted 1 line
see why we still need to call em28xx_audio_analog_set(). You will
notice
Quoted 2 lines
that I've rearranged the place where I update volume and mute. The rationale is that v4l2_device_call_all() might eventually change a
value
Quoted 3 lines
for volume/mute.
Another reason is that, IMO, v4l2_device_call_all() should return
values.
Quoted 1 line
In the specific case of volume/mute, if the user tries to specify a
value
Quoted 10 lines
outside the range, the -ERANGE should be returned.
I've already committed the patches at the tree. Please double-check.
Hans,
we need to fix the returned error value for v4l2_device_call_all(). I know that this is an old issue that weren't changed by v4l dev/subdev conversion, but now it is easier for us to fix. The idea here is to be sure that, if a sub-driver with a proper handling for a function
Quoted 8 lines
returns
Quoted 1 line
an error value, this would be returned by v4l2_device_call_all().
Maybe
Quoted 1 line
we'll need to adjust some things at the sub-drivers.
Use v4l2_device_call_until_err instead of v4l2_device_call_all. That macro
checks for errors returned from the subdevs.
It doesn't work as expected. If I use it for queryctl, for example, it returns
an empty set of controls. If I use it for g_ctrl, it returns:
error 22 getting ctrl Brightness error 22 getting ctrl Contrast error 22 getting ctrl Saturation error 22 getting ctrl Hue error 22 getting ctrl Volume error 22 getting ctrl Balance error 22 getting ctrl Bass error 22 getting ctrl Treble error 22 getting ctrl Mute error 22 getting ctrl Loudness
The issue here is we need something that discards errors for non-implemented
controls.
As the sub-drivers are returning -EINVAL for non-implemented controls (and probably other stuff that aren't implemented there), the function will not work
for some ioctls.
The proper fix seems to elect an error condition to be returned by driver when
a function is not implemented, and such errors to be discarded by the macro.

Quoted 8 lines

Quoted 3 lines
It seems that the proper error code for such case is this one:
#define ENOSYS 38 /* Function not implemented */
You are right, this macro doesn't work for these control functions. It it is possible to implement a define like ENOSYS, but I prefer to work on generic control processing code that is embedded in the v4l2 framework. It looks like I'll finally have time to work on that this weekend.

I did some tests here: if we replace -EINVAL with -ENOIOCTLCMD, we can properly make v4l2_device_call_until_err() to work, fixing the lack of a proper error report at the drivers. This error code seems also appropriate for this case.

This means several trivial patches on each v4l device driver, just replacing the error codes for 3 ioctl handlers (s_ctrl, g_ctrl, queryctrl).

I'll try to write such patches for v4l devices, since I want to get rid of this bug on 2.6.31, at least on em28xx driver. If I have more time, I'll fix other bridge drivers as well.

Quoted 3 lines

Currently the control handling code in our v4l drivers is, to be blunt, a pile of crap. And it is ideal to move this into the v4l2 framework since 90% of this is common code.

Hmm, except for a few places that still implement this at the old way, most of the common code is already at v4l2 core. So, I'm not sure what you're referring.

Cheers,
Mauro

Hans Verkuil

2009-07-29 15:08 UTC #8 lore raw

Quoted 9 lines

Em Wed, 29 Jul 2009 15:14:22 +0200
"Hans Verkuil" <[email protected]> escreveu:
Quoted 5 lines
Quoted 5 lines
Em Wed, 29 Jul 2009 08:09:31 +0200 Hans Verkuil <[email protected]> escreveu:
Quoted 1 line · On Wednesday 29 July 2009 06:57:30 Mauro Carvalho Chehab wrote:

<snip>

Quoted 62 lines

Quoted 52 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Hans,
we need to fix the returned error value for v4l2_device_call_all().
I
Quoted 1 line
Quoted 1 line
Quoted 1 line
know that this is an old issue that weren't changed by v4l
dev/subdev
Quoted 1 line
Quoted 1 line
Quoted 1 line
conversion, but now it is easier for us to fix. The idea here is to
be
Quoted 25 lines
Quoted 9 lines
Quoted 1 line
sure that, if a sub-driver with a proper handling for a function
returns
Quoted 1 line
an error value, this would be returned by v4l2_device_call_all().
Maybe
Quoted 1 line
we'll need to adjust some things at the sub-drivers.
Use v4l2_device_call_until_err instead of v4l2_device_call_all. That macro
checks for errors returned from the subdevs.
It doesn't work as expected. If I use it for queryctl, for example, it returns
an empty set of controls. If I use it for g_ctrl, it returns:
error 22 getting ctrl Brightness error 22 getting ctrl Contrast error 22 getting ctrl Saturation error 22 getting ctrl Hue error 22 getting ctrl Volume error 22 getting ctrl Balance error 22 getting ctrl Bass error 22 getting ctrl Treble error 22 getting ctrl Mute error 22 getting ctrl Loudness
Quoted 5 lines
The issue here is we need something that discards errors for non-implemented
controls.
As the sub-drivers are returning -EINVAL for non-implemented controls
(and
Quoted 1 line
probably other stuff that aren't implemented there), the function will
not
Quoted 4 lines
work
for some ioctls.
The proper fix seems to elect an error condition to be returned by
driver
Quoted 6 lines
when
a function is not implemented, and such errors to be discarded by the macro.
It seems that the proper error code for such case is this one:
Quoted 8 lines
Quoted 1 line
#define ENOSYS 38 /* Function not implemented */
You are right, this macro doesn't work for these control functions. It it
is possible to implement a define like ENOSYS, but I prefer to work on generic control processing code that is embedded in the v4l2 framework. It
looks like I'll finally have time to work on that this weekend.

Quoted 6 lines

I did some tests here: if we replace -EINVAL with -ENOIOCTLCMD, we can properly
make v4l2_device_call_until_err() to work, fixing the lack of a proper error
report at the drivers. This error code seems also appropriate for this case.

This is not sufficient: v4l2_device_call_until_err is not really suitable for this. This would be better:

#define __v4l2_device_call_subdevs_ctrls(v4l2_dev, cond, o, f, args...) \

({                                                                      \
        struct v4l2_subdev *sd;                                         \
        long err = 0;                                                   \
                                                                        \
        list_for_each_entry(sd, &(v4l2_dev)->subdevs, list) {           \
                if ((cond) && sd->ops->o && sd->ops->o->f)              \
                        err = sd->ops->o->f(sd , ##args);               \
                if (err && err != -ENOIOCTLCMD)                         \
                        break;                                          \
        }                                                               \
        (err == -ENOIOCTLCMD) ? -EINVAL : err;                          \
})

This way -EINVAL is returned if the control isn't handled anywhere.

Quoted 10 lines

This means several trivial patches on each v4l device driver, just replacing
the error codes for 3 ioctl handlers (s_ctrl, g_ctrl, queryctrl).
I'll try to write such patches for v4l devices, since I want to get rid of this
bug on 2.6.31, at least on em28xx driver. If I have more time, I'll fix other
bridge drivers as well.

Keep in mind that changing this for one i2c driver will mean that you have to check its behavior on all v4l2 drivers that use that i2c driver.

Quoted 9 lines

Quoted 4 lines
Currently the control handling code in our v4l drivers is, to be blunt, a
pile of crap. And it is ideal to move this into the v4l2 framework since 90% of this is common code.
Hmm, except for a few places that still implement this at the old way, most of
the common code is already at v4l2 core. So, I'm not sure what you're referring.

Just to name a few:

1) Range checking of the control values.
2) Generic handling of QUERYCTRL/QUERYMENU including standard support for the V4L2_CTRL_FLAG_NEXT_CTRL.
3) Generic handling of the VIDIOC_G/S/TRY_EXT_CTRLS ioctls so all drivers can handle them.
4) Controls that are handled in subdevs should be automatically detected by the core so that they are enumerated correctly.

There really is no support for this in the core, except for some utility functions in v4l2-common.c.

Regards,

Hans

Signature · 1 line

Hans Verkuil - video4linux developer - sponsored by TANDBERG

Mauro Carvalho Chehab

2009-07-30 03:06 UTC #9 lore raw

Em Wed, 29 Jul 2009 17:08:31 +0200
"Hans Verkuil" <[email protected]> escreveu:

Quoted 9 lines

Quoted 6 lines
I did some tests here: if we replace -EINVAL with -ENOIOCTLCMD, we can properly
make v4l2_device_call_until_err() to work, fixing the lack of a proper error
report at the drivers. This error code seems also appropriate for this case.
This is not sufficient: v4l2_device_call_until_err is not really suitable for this.

Agreed. Yet, the tests helped to see what changes were needed.

Quoted 11 lines

This would be better:

#define __v4l2_device_call_subdevs_ctrls(v4l2_dev, cond, o, f, args...) \

({                                                                      \
        struct v4l2_subdev *sd;                                         \
        long err = 0;                                                   \
                                                                        \
        list_for_each_entry(sd, &(v4l2_dev)->subdevs, list) {           \
                if ((cond) && sd->ops->o && sd->ops->o->f)              \
                        err = sd->ops->o->f(sd , ##args);               \
                if (err && err != -ENOIOCTLCMD)                         \

No, this is not right. In the case of controls, it should be, instead:

                 if (err != -ENOIOCTLCMD)

Also, such routine is has nothing specific for ctrls, so, the naming doesn't look nice. I'll find a better name.

Quoted 6 lines

                        break;                                          \
        }                                                               \
        (err == -ENOIOCTLCMD) ? -EINVAL : err;                          \

})

This way -EINVAL is returned if the control isn't handled anywhere.

Ok, but I'm in doubt if -EINVAL is the better return code on such case, but, in order to not break backward compatibility, better to return -EINVAL.

Quoted 5 lines

Quoted 5 lines
This means several trivial patches on each v4l device driver, just replacing
the error codes for 3 ioctl handlers (s_ctrl, g_ctrl, queryctrl).

Quoted 8 lines

Quoted 5 lines
I'll try to write such patches for v4l devices, since I want to get rid of this
bug on 2.6.31, at least on em28xx driver. If I have more time, I'll fix other
bridge drivers as well.
Keep in mind that changing this for one i2c driver will mean that you have to check its behavior on all v4l2 drivers that use that i2c driver.

As drivers currently don't check for the returned code (maybe except for a very few ones), this probably is not a big issue. Yet, I'll use a semantic check to generate the patches, to be sure that all cases were covered.

Quoted 20 lines

Quoted 9 lines
Quoted 4 lines
Currently the control handling code in our v4l drivers is, to be blunt, a
pile of crap. And it is ideal to move this into the v4l2 framework since 90% of this is common code.
Hmm, except for a few places that still implement this at the old way, most of
the common code is already at v4l2 core. So, I'm not sure what you're referring.
Just to name a few:
1) Range checking of the control values.
2) Generic handling of QUERYCTRL/QUERYMENU including standard support for the V4L2_CTRL_FLAG_NEXT_CTRL.
3) Generic handling of the VIDIOC_G/S/TRY_EXT_CTRLS ioctls so all drivers can handle them.
4) Controls that are handled in subdevs should be automatically detected by the core so that they are enumerated correctly.

Quoted 2 lines

There really is no support for this in the core, except for some utility functions in v4l2-common.c.

Ok.

Cheers,
Mauro

Hans Verkuil

2009-07-30 06:40 UTC #10 lore raw

Quoted 27 lines · On Thursday 30 July 2009 05:06:34 Mauro Carvalho Chehab wrote:

Em Wed, 29 Jul 2009 17:08:31 +0200
"Hans Verkuil" <[email protected]> escreveu:
Quoted 9 lines
Quoted 6 lines
I did some tests here: if we replace -EINVAL with -ENOIOCTLCMD, we can properly
make v4l2_device_call_until_err() to work, fixing the lack of a proper error
report at the drivers. This error code seems also appropriate for this case.
This is not sufficient: v4l2_device_call_until_err is not really suitable for this.
Agreed. Yet, the tests helped to see what changes were needed.
Quoted 11 lines
This would be better:
#define __v4l2_device_call_subdevs_ctrls(v4l2_dev, cond, o, f, args...) \
({                                                                      \
        struct v4l2_subdev *sd;                                         \
        long err = 0;                                                   \
                                                                        \
        list_for_each_entry(sd, &(v4l2_dev)->subdevs, list) {           \
                if ((cond) && sd->ops->o && sd->ops->o->f)              \
                        err = sd->ops->o->f(sd , ##args);               \
                if (err && err != -ENOIOCTLCMD)                         \

Quoted 3 lines

No, this is not right. In the case of controls, it should be, instead:
                 if (err != -ENOIOCTLCMD)

Indeed.

Quoted 3 lines

Also, such routine is has nothing specific for ctrls, so, the naming doesn't look nice. I'll find a better name.

I hacked it together on short notice, so feel to hack it some more :-)

Quoted 10 lines

Quoted 7 lines
                        break;                                          \
        }                                                               \
        (err == -ENOIOCTLCMD) ? -EINVAL : err;                          \
})
This way -EINVAL is returned if the control isn't handled anywhere.
Ok, but I'm in doubt if -EINVAL is the better return code on such case, but, in order to not break backward compatibility, better to return -EINVAL.

I'd love to be able to change the use of EINVAL for unknown controls. I think there are some other cases as well in the spec where EINVAL is incorrectly used. But that's almost impossible to change without breaking backwards compatibility.

The only way I can think of doing this is by having the application explicitly request such changed behavior. But that's a very slippery slope to go on.

Regards,

Hans

Signature · 1 line

Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom

[email protected]

author

2009-07-29 22:13 UTC #11 lore raw

In reply to Mauro Carvalho Chehab

Quoted 9 lines · Mauro Carvalho Chehab

Ah, yes, there's a missing mute/unmute issue there. Instead of using your code, I opted to duplicate part of ac97_set_ctrl code there.
I opted to have a small duplicated code, but, IMO, it is now clearer to see why we still need to call em28xx_audio_analog_set(). You will notice that I've rearranged the place where I update volume and mute. The rationale is that v4l2_device_call_all() might eventually change a value for volume/mute.

Quoted 6 lines

Another reason is that, IMO, v4l2_device_call_all() should return values. In the specific case of volume/mute, if the user tries to specify a value outside the range, the -ERANGE should be returned.
I've already committed the patches at the tree. Please double-check.

It doesn't work. Mplayer locks up. There's no video window, but sound works. The only way to kill mplayer is rebooting the machine.

Mauro Carvalho Chehab

2009-07-30 04:06 UTC #12 lore raw

Em Wed, 29 Jul 2009 18:13:36 -0400
[email protected] escreveu:

Quoted 14 lines · Mauro Carvalho Chehab

Quoted 13 lines
Ah, yes, there's a missing mute/unmute issue there. Instead of using your code, I opted to duplicate part of ac97_set_ctrl code there.
I opted to have a small duplicated code, but, IMO, it is now clearer to see why we still need to call em28xx_audio_analog_set(). You will notice that I've rearranged the place where I update volume and mute. The rationale is that v4l2_device_call_all() might eventually change a value for volume/mute.
Another reason is that, IMO, v4l2_device_call_all() should return values. In the specific case of volume/mute, if the user tries to specify a value outside the range, the -ERANGE should be returned.

Quoted 5 lines

Quoted 2 lines
I've already committed the patches at the tree. Please double-check.
It doesn't work. Mplayer locks up. There's no video window, but sound works. The only way to kill mplayer is rebooting the machine.

Hmm... a small mistake that kept mutex locked. The enclosed patch should fix.

Cheers,
Mauro