Hans,
Thanks for reviewing this. I have some questions though against your comments. Please see below for details..
Murali Karicheri
Software Design Engineer
Texas Instruments Inc.
Germantown, MD 20874
email: m-karicheri2@xxxxxx
>First of all I've reviewed the other patches in this series and they look
>OK
>to me. So you can add
>Reviewed-by: Hans Verkuil <hverkuil@xxxxxxxxx>
>for patches 1, 2, 3 and 5.
>
Will do.
>> + ret = vpif_check_format(ch, &common->fmt.fmt.pix, 0);
>> + if (ret)
>> + return ret;
>> +
>> + /* Enable streamon on the sub device */
>> + ret = v4l2_device_call_until_err(&vpif_obj.v4l2_dev,
>> + vpif_obj.sd[ch->curr_sd_index]->grp_id,
>> + video, s_stream, 1);
>> +
>> + if (ret && (ret != -ENOIOCTLCMD)) {
>> + vpif_dbg(1, debug, "stream on failed in subdev\n");
>> + return ret;
>> + }
>
>I strongly recommend that instead of calling the subdev for the current
>input
>indirectly using v4l2_device_call_until_err() you use the v4l2_subdev
>pointer
>associated with the current input directly. So typically when the input is
>changed you update a ch->curr_sd pointer to the associated struct
>v4l2_subdev.
>
>After that you can just call v4l2_subdev_call(ch->sd, video, s_stream, 1).
>Much more concise.
>
>In addition the v4l2_device_call_until_err() macro will replace the
>ENOIOCTLCMD
>error by 0. The idea behind this macro is that you have an unknown number
>of
>subdevs (>= 0) that might be interested in this command, but if no one is,
>then
>that's fine too. So in the code above the extra check to see whether the
>return code was -ENOIOCTLCMD is not needed in the case of
>v4l2_device_call_until_err(). But it is needed if you switch to
>v4l2_subdev_call().
>
So in short what you are suggesting is to replace all instances of v4l2_device_call_until_err() with v4l2_subdev_call() since after input selection we know exactly which sub device to direct the application
request to.
>> + .fops = &vpif_fops,
>> + .minor = -1,
>> + .ioctl_ops = &vpif_ioctl_ops,
>> + .current_norm = V4L2_STD_625_50,
>
>No need to set current_norm since it's overridden by g_std.
>
You mean s_std() right?
>Note: I've just found a bug in the default handling of VIDIOC_G_PARM in
>v4l2-ioctl.c since that uses current_norm even when g_std is defined.
>I will make a fix for that. As a general remark I have to say that I
>never liked that v4l2-ioctl has default handling for g_std. It's a
>dangerous
>construction that will definitely fail whenever you have both video and vbi
>device nodes.
>
Ok. Understood.
So I will make the next set of patches with the changes suggested by you and It would be ready for merge to your tree as well as to v4l-dvb linux-next tree (through your pull request to Mauro)
Thanks and regards,
Murali
--
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
