On Sun, Oct 2, 2011 at 6:30 PM, Sakari Ailus <sakari.ailus@xxxxxx> wrote:
> Hi Javier,
>
> Thanks for the patch! It's very interesting to see a driver for a video
> decoder using the MC interface. Before this we've had just image sensors.
>
Hello Sakari,
Thanks for your comments.
> Javier Martinez Canillas wrote:
>> + /* use the standard status register */
>> + std_status = tvp5150_read(sd, TVP5150_STATUS_REG_5);
>> + else
>> + /* use the standard register itself */
>> + std_status = std;
>
> Braces would be nice here.
>
Ok.
>> + switch (std_status & VIDEO_STD_MASK) {
>> + case VIDEO_STD_NTSC_MJ_BIT:
>> + case VIDEO_STD_NTSC_MJ_BIT_AS:
>> + return STD_NTSC_MJ;
>> +
>> + case VIDEO_STD_PAL_BDGHIN_BIT:
>> + case VIDEO_STD_PAL_BDGHIN_BIT_AS:
>> + return STD_PAL_BDGHIN;
>> +
>> + default:
>> + return STD_INVALID;
>> + }
>> +
>> + return STD_INVALID;
>
> This return won't do anything.
>
Yes, will clean this.
>> @@ -704,19 +812,19 @@ static int tvp5150_set_std(struct v4l2_subdev *sd, v4l2_std_id std)
>> if (std == V4L2_STD_ALL) {
>> fmt = 0; /* Autodetect mode */
>> } else if (std & V4L2_STD_NTSC_443) {
>> - fmt = 0xa;
>> + fmt = VIDEO_STD_NTSC_4_43_BIT;
>> } else if (std & V4L2_STD_PAL_M) {
>> - fmt = 0x6;
>> + fmt = VIDEO_STD_PAL_M_BIT;
>> } else if (std & (V4L2_STD_PAL_N | V4L2_STD_PAL_Nc)) {
>> - fmt = 0x8;
>> + fmt = VIDEO_STD_PAL_COMBINATION_N_BIT;
>> } else {
>> /* Then, test against generic ones */
>> if (std & V4L2_STD_NTSC)
>> - fmt = 0x2;
>> + fmt = VIDEO_STD_NTSC_MJ_BIT;
>> else if (std & V4L2_STD_PAL)
>> - fmt = 0x4;
>> + fmt = VIDEO_STD_PAL_BDGHIN_BIT;
>> else if (std & V4L2_STD_SECAM)
>> - fmt = 0xc;
>> + fmt = VIDEO_STD_SECAM_BIT;
>> }
>
> Excellent! Less magic numbers...
>
>>
>> +static struct v4l2_mbus_framefmt *
>> +__tvp5150_get_pad_format(struct tvp5150 *tvp5150, struct v4l2_subdev_fh *fh,
>> + unsigned int pad, enum v4l2_subdev_format_whence which)
>> +{
>> + switch (which) {
>> + case V4L2_SUBDEV_FORMAT_TRY:
>> + return v4l2_subdev_get_try_format(fh, pad);
>> + case V4L2_SUBDEV_FORMAT_ACTIVE:
>> + return tvp5150->format;
>> + default:
>> + return NULL;
>
> Hmm. This will never happen, but is returning NULL the right thing to
> do? An easy alternative is to just replace this with if (which may only
> have either of the two values).
>
Ok I'll cleanup, I was being a bit paranoid there :)
>> +
>> +static int tvp5150_set_pad_format(struct v4l2_subdev *subdev,
>> + struct v4l2_subdev_fh *fh,
>> + struct v4l2_subdev_format *format)
>> +{
>> + struct tvp5150 *tvp5150 = to_tvp5150(subdev);
>> + tvp5150->std_idx = STD_INVALID;
>
> The above assignment will always be overwritten immediately.
>
Yes, since tvp515x_query_current_std() already returns STD_INVALID on
error the assignment is not needed. Will change that.
>> + tvp5150->std_idx = tvp515x_query_current_std(subdev);
>> + if (tvp5150->std_idx == STD_INVALID) {
>> + v4l2_err(subdev, "Unable to query std\n");
>> + return 0;
>
> Isn't this an error?
>
Yes, I'll change to report the error to the caller.
>> + * tvp515x_mbus_fmt_cap() - V4L2 decoder interface handler for try/s/g_mbus_fmt
>
> The name of the function is different.
>
Yes, I'll change that.
>> static const struct v4l2_subdev_video_ops tvp5150_video_ops = {
>> .s_routing = tvp5150_s_routing,
>> + .s_stream = tvp515x_s_stream,
>> + .enum_mbus_fmt = tvp515x_enum_mbus_fmt,
>> + .g_mbus_fmt = tvp515x_mbus_fmt,
>> + .try_mbus_fmt = tvp515x_mbus_fmt,
>> + .s_mbus_fmt = tvp515x_mbus_fmt,
>> + .g_parm = tvp515x_g_parm,
>> + .s_parm = tvp515x_s_parm,
>> + .s_std_output = tvp5150_s_std,
>
> Do we really need both video and pad format ops?
>
Good question, I don't know. Can this device be used as a standalone
v4l2 device? Or is supposed to always be a part of a video streaming
pipeline as a sub-device with a source pad? Sorry if my questions are
silly but as I stated before, I'm a newbie with v4l2 and MCF.
> s_std should be added to pad ops so it would be available on the subdev
> node.
>
Ok, I'll add s_std operation.
>>
>> +static int tvp515x_enum_mbus_code(struct v4l2_subdev *subdev,
>> + struct v4l2_subdev_fh *fh,
>> + struct v4l2_subdev_mbus_code_enum *code)
>> +{
>> + if (code->index >= ARRAY_SIZE(tvp515x_std_list))
>> + return -EINVAL;
>> +
>> + code->code = V4L2_MBUS_FMT_UYVY8_2X8;
>
> If there's just one supported mbus code, non-zero code->index must
> return -EINVAL.
>
Ok, I'll change that.
>> + return 0;
>> +}
>> +
>> +static int tvp515x_enum_frame_size(struct v4l2_subdev *subdev,
>> + struct v4l2_subdev_fh *fh,
>> + struct v4l2_subdev_frame_size_enum *fse)
>> +{
>> + int current_std = STD_INVALID;
>
> current_std is overwritten before it gets used.
>
Yes, same case here, will remove the assignment.
>> + if (fse->code != V4L2_MBUS_FMT_UYVY8_2X8)
>> + return -EINVAL;
>> +
>> + /* query the current standard */
>> + current_std = tvp515x_query_current_std(subdev);
>> + if (current_std == STD_INVALID) {
>> + v4l2_err(subdev, "Unable to query std\n");
>> + return 0;
>> + }
>
> I wonder how the enum_frame_size and s_std are supposed to interact,
> especially that I understand, after reading the discussion, the chip may
> be used to force certain standard while the actual signal is different.
>
Well my thought was that the application can select which standard
(i.e: V4L2_STD_PAL) and enum_frame_size will return the width and
height for that standard (i.e: 720x625 for PAL). Since that is what
the device is capturing.
Then a user-space application can configure the CCDC and RESIZER to
modify the format. Does this make sense to you? Or the user-space
application can select a different frame size at the sub-dev level
(tvp5151)?
>
> --
> Sakari Ailus
> sakari.ailus@xxxxxx
>
Thanks a lot for your suggestions and comments.
Best regards,
--
Javier Martínez Canillas
(+34) 682 39 81 69
Barcelona, Spain
--
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
