Hans,
I am not clear about some of your comments. Please see below with a [MK] prefix.
>> +static int debug;
>> +static u32 numbuffers = 3;
>> +static u32 bufsize = (720 * 576 * 2);
>> +
>> +module_param(numbuffers, uint, S_IRUGO);
>> +module_param(bufsize, uint, S_IRUGO);
>> +module_param(debug, int, 0644);
>> +
>> +
>> +/* Set interface params based on client interface */
>> +static int vpfe_set_hw_if_params(struct vpfe_device *vpfe_dev)
>> +{
>> + struct vpfe_subdev_info *subdev = vpfe_dev->current_subdev;
>> + struct v4l2_routing *route =
>> + &(subdev->routes[vpfe_dev->current_input]);
>> +
>> + switch (route->output) {
>> + case OUTPUT_10BIT_422_EMBEDDED_SYNC:
>> + vpfe_dev->vpfe_if_params.if_type = VPFE_BT656;
>> + break;
>> + case OUTPUT_20BIT_422_SEPERATE_SYNC:
>> + vpfe_dev->vpfe_if_params.if_type = VPFE_YCBCR_SYNC_16;
>> + break;
>> + case OUTPUT_10BIT_422_SEPERATE_SYNC:
>> + vpfe_dev->vpfe_if_params.if_type = VPFE_YCBCR_SYNC_8;
>> + break;
>> + default:
>> + v4l2_err(&vpfe_dev->v4l2_dev, "decoder output"
>> + " not supported, %d\n", route->output);
>> + return -EINVAL;
>> + }
>> +
>> + /* set if client specific interface param is available */
>> + if (subdev->pdata) {
>> + /* each client will have different interface requirements */
>> + if (!strcmp(subdev->name, "tvp5146")) {
>> + struct tvp514x_platform_data *pdata = subdev->pdata;
>> +
>> + if (pdata->hs_polarity)
>> + vpfe_dev->vpfe_if_params.hdpol =
>> + VPFE_PINPOL_POSITIVE;
>> + else
>> + vpfe_dev->vpfe_if_params.hdpol =
>> + VPFE_PINPOL_NEGATIVE;
>> +
>> + if (pdata->vs_polarity)
>> + vpfe_dev->vpfe_if_params.vdpol =
>> + VPFE_PINPOL_POSITIVE;
>> + else
>> + vpfe_dev->vpfe_if_params.hdpol =
>> + VPFE_PINPOL_NEGATIVE;
>
>This won't work. Instead this should be data associated with the
>platform_data.
>I.e. the platform_data for the dm355/dm6446 contains not only the subdev
>information, but for each subdev also the information on how to setup the
>vpfe
>polarities. You cannot derive that information from what subdevs are used
>since
>the board designer might have added e.g. inverters or something like that.
>Such
>information can only come from the platform_data.
>
[MK] I know this code is not correct. But I was waiting for the discussion on my bus parameter patch to make this change. Currently TVP514x driver that you have reviewed configure output bus based on route->output parameter set by s_route(). This doesn't make sense. The input param make sense since application can choose between Composite and S-Video inputs. There is only one bus going out of the sub device to vpfe. So the output selection @ sub device is redundant. I think the output is part of as the bus parameter structure I added in the bus parameter patch which is under review. It can be read by TVP514x from the platform data (using the structure added by my patch) and can be overridden by s_bus(). Do you expect the bridge driver and sub devices having platform data for bus type (For example, BT.656)? It appears to be required only for sub device and bridge driver can configure the ccdc based on sub device bus type. But for polarities I need to define them for both sides. comments?
>> + } else {
>> + v4l2_err(&vpfe_dev->v4l2_dev, "No interface params"
>> + " defined for subdevice, %d\n", route->output);
>> + return -EFAULT;
>> + }
>> + }
>> + return ccdc_dev->hw_ops.set_hw_if_params(&vpfe_dev->vpfe_if_params);
>> +}
>> +
>> +/*
>> +
>> + struct vpfe_fh *fh;
>> +
>> + v4l2_dbg(1, debug, &vpfe_dev->v4l2_dev, "vpfe_open\n");
>> +
>> + if (!vpfe_dev->cfg->num_subdevs) {
>> + v4l2_err(&vpfe_dev->v4l2_dev, "No decoder registered\n");
>> + return -ENODEV;
>> + }
>
>Why would this be an error? I might have an FPGA connected instead or some
>other non-i2c device that doesn't require any setup from this driver.
>
[MK] What you mean by this? Are you saying an FPGA logic will implement the decoder hardware? That is quite possible, so also it could be non-i2c. But my understanding was that sub device can be anything that basically implement the sub device API and need not always be an i2c device. So for FPGA or some other bus based device, the bridge device doesn't care how the command to change input, detect standard etc are communicated by the sub device driver to its hardware. It could be writing into some FPGA register or sending a proprietary protocol command. Is my understanding correct? In that case each of the above (FPGA or non-i2c) is a sub device and at least one sub device should be available before application can do any useful operation with the capture device. So the check is required. Am I missing something here?
>
>> +
>> +unlock_out:
>> + mutex_unlock(&vpfe_dev->lock);
>> + return ret;
>> +}
>> +
>> +
>> +static int vpfe_queryctrl(struct file *file, void *priv,
>> + struct v4l2_queryctrl *qc)
>> +{
>> + struct vpfe_device *vpfe_dev = video_drvdata(file);
>> + struct vpfe_subdev_info *sub_dev = vpfe_dev->current_subdev;
>> +
>> + v4l2_dbg(1, debug, &vpfe_dev->v4l2_dev, "vpfe_queryctrl\n");
>> +
>> + if (qc->id >= V4L2_CID_PRIVATE_BASE) {
>> + /* It is ccdc CID */
>> + if (ccdc_dev->hw_ops.queryctrl)
>> + return ccdc_dev->hw_ops.queryctrl(qc);
>> + }
>> + /* pass it to sub device */
>> + return v4l2_device_call_until_err(&vpfe_dev->v4l2_dev, sub_dev-
>>grp_id,
>> + core, queryctrl, qc);
>> +}
>> +
>> +static int vpfe_g_ctrl(struct file *file, void *priv,
>> + struct v4l2_control *ctrl)
>> +{
>> + struct vpfe_device *vpfe_dev = video_drvdata(file);
>> + struct vpfe_subdev_info *sub_dev = vpfe_dev->current_subdev;
>> +
>> + v4l2_dbg(1, debug, &vpfe_dev->v4l2_dev, "vpfe_g_ctrl\n");
>> +
>> + if (ctrl->id >= V4L2_CID_PRIVATE_BASE) {
>> + /* It is ccdc CID */
>> + if (ccdc_dev->hw_ops.get_control)
>> + return ccdc_dev->hw_ops.get_control(ctrl);
>> + }
>
>Don't use these PRIVATE_BASE controls. See also this post regarding
>the current situation regarding private controls:
>
>http://www.mail-archive.com/linux-omap%40vger.kernel.org/msg07999.html
>
[MK] Looks like it is better to add it to TBD and address it when I add camera interface support.
>--
>Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom
--
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
