Em 12-06-2011 14:27, Mauro Carvalho Chehab escreveu:
> Em 12-06-2011 13:07, Hans Verkuil escreveu:
>> On Sunday, June 12, 2011 16:37:55 Mauro Carvalho Chehab wrote:
>>> Em 12-06-2011 07:59, Hans Verkuil escreveu:
>>>> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>>>
>>>> The check_mode function checks whether a mode is supported. So calling it
>>>> supported_mode is more appropriate. In addition it returned either 0 or
>>>> -EINVAL which suggests that the -EINVAL error should be passed on. However,
>>>> that's not the case so change the return type to bool.
>>>
>>> I prefer to keep returning -EINVAL. This is the proper thing to do, and
>>> to return the result to the caller. A fixme should be added though, so,
>>> after someone add a subdev call that would properly handle the -EINVAL
>>> code for multiple tuners, the functions should return the error code
>>> instead of 0.
>>
>> No, you can't return -EINVAL here. It is the responsibility of the bridge
>> driver to determine whether there is e.g. a radio tuner. So if one of these
>> tuner subdevs is called with mode radio while it is a tv tuner, then that
>> is not an error, but instead it is a request that can safely be ignored
>> as not relevant for that tuner. It is up to the bridge driver to ensure
>> that a tuner is loaded that is actually valid for the radio mode.
>>
>> Subdev ops should only return errors when there is a real problem (e.g. i2c
>> errors) and should just return 0 if a request is not for them.
>>
>> That's why I posted these first two patches: these functions suggest that you
>> can return an error if the mode doesn't match when you really cannot.
>>
>> If I call v4l2_device_call_until_err() for e.g. s_frequency, then the error
>> that is returned should match a real error (e.g. an i2c error), not that one
>> of the tv tuners refused the radio mode. I know there is a radio tuner somewhere,
>> otherwise there wouldn't be a /dev/radio node.
>>
>> I firmly believe that the RFCv4 series is correct and just needs an additional
>> patch adding some documentation.
>>
>> That said, it would make sense to move the first three patches to the end
>> instead if you prefer. Since these are cleanups, not bug fixes like the others.
>
>
> The errors at tuner should be propagated. If there's just one tuner, the error
> code should just be returned by the ioctl. But, if there are two tuners, if the bridge
> driver calls G_TUNER (or any other tuner subdev call) and both tuners return -EINVAL,
> then it needs to return -EINVAL to userspace. If just one returns -EINVAL, and the
> other tuner returns 0, then it should return 0. So, it is about the opposite behaviour
> implemented at v4l2_device_call_until_err().
>
> In order to implement the correct behaviour, the tuner driver should return -EINVAL if
> check_mode/set_mode fails. However, this breaks any bridge that may be using
> v4l2_device_call_until_err(). That's why the current code returns 0.
>
> The proper fix for it is:
>
> 1) create a call_all function that returns 0 if one of the subdevs returned 0,
> or returns an error code otherwise;
> 2) change all bridge calls to tuner stuff to the new call_all function;
> 3) return the check_mode/set_mode error to the bridge.
>
> One alternative for (1) would be to simply change the v4l2_device_call_all() to return 0 if
> one of the subdrivers returned 0. Something like (not tested):
>
Sorry, wrong logic. It should be, instead:
#define __v4l2_device_call_subdevs_p(v4l2_dev, sd, cond, o, f, args..$
({ \
long __rc = -ENOIOCTLCMD, __err = _rc; \
\
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) \
__rc = 0; \
} \
} \
(__rc == 0) ? 0 : __err; \
})
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
