[PATCH] em28xx: enable usb audio for plextor px-tv100u

[PATCH] em28xx: enable usb audio for plextor px-tv100u

[acano]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: 2009-07-18-01-plextor.patch --]
[-- Type: text/x-diff, Size: 1702 bytes --]

diff -r 27ddf3fe0ed9 linux/drivers/media/video/em28xx/em28xx-cards.c
--- a/linux/drivers/media/video/em28xx/em28xx-cards.c	Wed Jun 17 04:38:12 2009 +0000
+++ b/linux/drivers/media/video/em28xx/em28xx-cards.c	Sat Jul 18 13:32:04 2009 -0400
@@ -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:
diff -r 27ddf3fe0ed9 linux/drivers/media/video/em28xx/em28xx-video.c
--- a/linux/drivers/media/video/em28xx/em28xx-video.c	Wed Jun 17 04:38:12 2009 +0000
+++ b/linux/drivers/media/video/em28xx/em28xx-video.c	Sat Jul 18 13:32:04 2009 -0400
@@ -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) {

Re: [PATCH] em28xx: enable usb audio for plextor px-tv100u

[Mauro Carvalho Chehab]

Hi Acano,

Em Sat, 18 Jul 2009 13:37:58 -0400
acano@fastmail.fm escreveu:

> @@ -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);

This is legacy. While here, it is better to move the gpio setup it to the
proper place.

> +
> +		/* enable audio 12mhz i2s */
> +		em28xx_write_reg(dev, EM28XX_R0F_XCLK, 0xa7);

Instead of writing directly at xclk, the best is to initialize its value at
the boards struct.

> +		dev->i2s_speed = 2048000;
>  		break;
>  	case EM2861_BOARD_KWORLD_PVRTV_300U:
>  	case EM2880_BOARD_KWORLD_DVB_305U:
> diff -r 27ddf3fe0ed9 linux/drivers/media/video/em28xx/em28xx-video.c
> --- a/linux/drivers/media/video/em28xx/em28xx-video.c	Wed Jun 17 04:38:12 2009 +0000
> +++ b/linux/drivers/media/video/em28xx/em28xx-video.c	Sat Jul 18 13:32:04 2009 -0400
> @@ -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);

Hmm... this function were removed. In thesis, you shouldn't need to do anything
to unmute.

> +
>  		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) {

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.

Cheers,
Mauro

em28xx: fix audio on Plextor PX-TV100U

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>


diff --git a/linux/drivers/media/video/em28xx/em28xx-cards.c b/linux/drivers/media/video/em28xx/em28xx-cards.c
--- a/linux/drivers/media/video/em28xx/em28xx-cards.c
+++ b/linux/drivers/media/video/em28xx/em28xx-cards.c
@@ -639,22 +639,27 @@ 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,
+		.xclk         = EM28XX_XCLK_I2S_MSB_TIMING |
+				EM28XX_XCLK_FREQUENCY_12MHZ,
 		.tda9887_conf = TDA9887_PRESENT,
 		.decoder      = EM28XX_TVP5150,
+		.has_msp34xx  = 1,
 		.input        = { {
 			.type     = EM28XX_VMUX_TELEVISION,
 			.vmux     = TVP5150_COMPOSITE0,
 			.amux     = EM28XX_AMUX_LINE_IN,
+			.gpio     = pinnacle_hybrid_pro_analog,
 		}, {
 			.type     = EM28XX_VMUX_COMPOSITE1,
 			.vmux     = TVP5150_COMPOSITE1,
 			.amux     = EM28XX_AMUX_LINE_IN,
+			.gpio     = pinnacle_hybrid_pro_analog,
 		}, {
 			.type     = EM28XX_VMUX_SVIDEO,
 			.vmux     = TVP5150_SVIDEO,
 			.amux     = EM28XX_AMUX_LINE_IN,
+			.gpio     = pinnacle_hybrid_pro_analog,
 		} },
 	},
 
@@ -1948,9 +1953,8 @@ void em28xx_pre_card_setup(struct em28xx
 	/* request some modules */
 	switch (dev->model) {
 	case EM2861_BOARD_PLEXTOR_PX_TV100U:
-		/* FIXME guess */
-		/* Turn on analog audio output */
-		em28xx_write_reg(dev, EM28XX_R08_GPIO, 0xfd);
+		/* Sets the msp34xx I2S speed */
+		dev->i2s_speed = 2048000;
 		break;
 	case EM2861_BOARD_KWORLD_PVRTV_300U:
 	case EM2880_BOARD_KWORLD_DVB_305U:




Cheers,
Mauro

Re: [PATCH] em28xx: enable usb audio for plextor px-tv100u

[acano]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: multipart/mixed; boundary="tKW2IUtsqtDRztdT", Size: 0 bytes --]

Re: [PATCH] em28xx: enable usb audio for plextor px-tv100u

[Mauro Carvalho Chehab]

Em Tue, 28 Jul 2009 20:07:53 -0400
acano@fastmail.fm escreveu:

> On Mon, Jul 27, 2009 at 09:28:11PM -0300, Mauro Carvalho Chehab wrote:
> > Hi Acano,
> 
> Tested-by: Angelo Cano <acano@fastmail.fm>
> 
> works great

Good!

> > > +		/*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.
> 
> >
> > 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.
> >
> 
> 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

Re: [PATCH] em28xx: enable usb audio for plextor px-tv100u

[Hans Verkuil]

On Wednesday 29 July 2009 06:57:30 Mauro Carvalho Chehab wrote:
> Em Tue, 28 Jul 2009 20:07:53 -0400
>
> acano@fastmail.fm escreveu:
> > On Mon, Jul 27, 2009 at 09:28:11PM -0300, Mauro Carvalho Chehab wrote:
> > > Hi Acano,
> >
> > Tested-by: Angelo Cano <acano@fastmail.fm>
> >
> > works great
>
> Good!
>
> > > > +		/*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.
> >
> > > 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.
> >
> > 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.

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

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom

Re: [PATCH] em28xx: enable usb audio for plextor px-tv100u

[Mauro Carvalho Chehab]

Em Wed, 29 Jul 2009 08:09:31 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On Wednesday 29 July 2009 06:57:30 Mauro Carvalho Chehab wrote:
> > Em Tue, 28 Jul 2009 20:07:53 -0400
> >
> > acano@fastmail.fm escreveu:
> > > On Mon, Jul 27, 2009 at 09:28:11PM -0300, Mauro Carvalho Chehab wrote:
> > > > Hi Acano,
> > >
> > > Tested-by: Angelo Cano <acano@fastmail.fm>
> > >
> > > works great
> >
> > Good!
> >
> > > > > +		/*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.
> > >
> > > > 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.
> > >
> > > 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.
> 
> 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

Re: [PATCH] em28xx: enable usb audio for plextor px-tv100u

[Hans Verkuil]

> Em Wed, 29 Jul 2009 08:09:31 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>
>> On Wednesday 29 July 2009 06:57:30 Mauro Carvalho Chehab wrote:
>> > Em Tue, 28 Jul 2009 20:07:53 -0400
>> >
>> > acano@fastmail.fm escreveu:
>> > > On Mon, Jul 27, 2009 at 09:28:11PM -0300, Mauro Carvalho Chehab
>> wrote:
>> > > > Hi Acano,
>> > >
>> > > Tested-by: Angelo Cano <acano@fastmail.fm>
>> > >
>> > > works great
>> >
>> > Good!
>> >
>> > > > > +		/*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.
>> > >
>> > > > 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.
>> > >
>> > > 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.
>>
>> 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 */

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

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [PATCH] em28xx: enable usb audio for plextor px-tv100u

[Mauro Carvalho Chehab]

Em Wed, 29 Jul 2009 15:14:22 +0200
"Hans Verkuil" <hverkuil@xs4all.nl> escreveu:

> 
> > Em Wed, 29 Jul 2009 08:09:31 +0200
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >
> >> On Wednesday 29 July 2009 06:57:30 Mauro Carvalho Chehab wrote:
> >> > Em Tue, 28 Jul 2009 20:07:53 -0400
> >> >
> >> > acano@fastmail.fm escreveu:
> >> > > On Mon, Jul 27, 2009 at 09:28:11PM -0300, Mauro Carvalho Chehab
> >> wrote:
> >> > > > Hi Acano,
> >> > >
> >> > > Tested-by: Angelo Cano <acano@fastmail.fm>
> >> > >
> >> > > works great
> >> >
> >> > Good!
> >> >
> >> > > > > +		/*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.
> >> > >
> >> > > > 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.
> >> > >
> >> > > 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.
> >>
> >> 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 */
> 
> 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.

> 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

Re: [PATCH] em28xx: enable usb audio for plextor px-tv100u

[Hans Verkuil]

> Em Wed, 29 Jul 2009 15:14:22 +0200
> "Hans Verkuil" <hverkuil@xs4all.nl> escreveu:
>
>>
>> > Em Wed, 29 Jul 2009 08:09:31 +0200
>> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>> >
>> >> On Wednesday 29 July 2009 06:57:30 Mauro Carvalho Chehab wrote:

<snip>

>> >> > 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.
>> >
>> > 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 */
>>
>> 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 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.

>
> 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.

>> 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

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [PATCH] em28xx: enable usb audio for plextor px-tv100u

[Mauro Carvalho Chehab]

Em Wed, 29 Jul 2009 17:08:31 +0200
"Hans Verkuil" <hverkuil@xs4all.nl> escreveu:

> > 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.

>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.

>                         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.

> >
> > 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.

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.

> >> 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.

Ok.



Cheers,
Mauro

Re: [PATCH] em28xx: enable usb audio for plextor px-tv100u

[Hans Verkuil]

On Thursday 30 July 2009 05:06:34 Mauro Carvalho Chehab wrote:
> Em Wed, 29 Jul 2009 17:08:31 +0200
> "Hans Verkuil" <hverkuil@xs4all.nl> escreveu:
> 
> > > 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.
> 
> >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)

Indeed.

> 
> 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 :-)

> 
> >                         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

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom

Re: [PATCH] em28xx: enable usb audio for plextor px-tv100u

[acano]

On Wed, Jul 29, 2009 at 01:57:30AM -0300, Mauro Carvalho Chehab wrote:
> 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.
>

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.

Re: [PATCH] em28xx: enable usb audio for plextor px-tv100u

[Mauro Carvalho Chehab]

Em Wed, 29 Jul 2009 18:13:36 -0400
acano@fastmail.fm escreveu:

> On Wed, Jul 29, 2009 at 01:57:30AM -0300, Mauro Carvalho Chehab wrote:
> > 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.
> >
> 
> 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