| Message ID | 20090729000753.GA24496@localhost.localdomain (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Mauro Carvalho Chehab |
| Headers | show |
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 -- 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 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
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 -- 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
> 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
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 -- 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
> 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
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. -- 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
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 -- 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
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 -- 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 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
diff -r fd96af63f79b linux/drivers/media/video/em28xx/em28xx-cards.c --- a/linux/drivers/media/video/em28xx/em28xx-cards.c Fri Jun 19 19:56:56 2009 +0000 +++ b/linux/drivers/media/video/em28xx/em28xx-cards.c Tue Jul 28 19:26:58 2009 -0400 @@ -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: diff -r fd96af63f79b linux/drivers/media/video/em28xx/em28xx-video.c --- a/linux/drivers/media/video/em28xx/em28xx-video.c Fri Jun 19 19:56:56 2009 +0000 +++ b/linux/drivers/media/video/em28xx/em28xx-video.c Tue Jul 28 19:26:58 2009 -0400 @@ -1123,8 +1123,32 @@ static int vidioc_s_ctrl(struct file *fi else rc = 1; - /* It were not an AC97 control. Sends it to the v4l2 dev interface */ + /* It we're not an AC97 control. Sends it to the v4l2 dev interface */ if (rc == 1) { + /* hack ac97_set_ctrl() call to unmute usb audio for Plextor ConvertX + * PX-TV100U + * + * need to eventually reach em28xx_audio_analog_set() + * + * and: + ***************** + * xclk = dev->board.xclk & 0x7f; + * if (!dev->mute) + * xclk |= EM28XX_XCLK_AUDIO_UNMUTE; + * + * ret = em28xx_write_reg(dev, EM28XX_R0F_XCLK, xclk); + * if (ret < 0) + * return ret; + * msleep(10); + ***************** + * + * included here as a simple call ac97_set_ctrl() since the path to + * em28xx_audio_analog_set() has the necessary conditional checks + * which I don't want to bother duplicating, and in case I needed + * something else besides unmuting. + */ + ac97_set_ctrl(dev, ctrl); + v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_ctrl, ctrl); /* FIXME: should be returning a meaninful value */ rc = 0;