Re: [RFC PATCH 1/3] adv7842: add new video decoder driver.

[Hans Verkuil <hverkuil@xs4all.nl>]

On 08/16/2013 12:10 PM, Scott Jiang wrote:
>> +
>> +static int adv7842_g_mbus_fmt(struct v4l2_subdev *sd,
>> +                             struct v4l2_mbus_framefmt *fmt)
>> +{
>> +       struct adv7842_state *state = to_state(sd);
>> +
>> +       fmt->width = state->timings.bt.width;
>> +       fmt->height = state->timings.bt.height;
>> +       fmt->code = V4L2_MBUS_FMT_FIXED;
>> +       fmt->field = V4L2_FIELD_NONE;
>> +
>> +       if (state->mode == ADV7842_MODE_SDP) {
>> +               /* SPD block */
>> +               if (!(sdp_read(sd, 0x5A) & 0x01))
>> +                       return -EINVAL;
>> +               fmt->width = 720;
>> +               /* valid signal */
>> +               if (state->norm & V4L2_STD_525_60)
>> +                       fmt->height = 480;
>> +               else
>> +                       fmt->height = 576;
>> +               fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
>> +               return 0;
>> +       }
>> +
> I believe someone use SDP mode to capture 480i instead of 480p.
> I think we can add a table to map adv7842 output setting and v4l format.

The driver as it stands only supports progressive output from the chip, because
that is all we support in our products (interlaced video conferencing? Nah, not
a good idea...).

I don't think I can easily test it (if at all!), so I leave this as an
exercise for the reader.

>> +static int adv7842_s_std(struct v4l2_subdev *sd, v4l2_std_id norm)
>> +{
>> +       struct adv7842_state *state = to_state(sd);
>> +
>> +       v4l2_dbg(1, debug, sd, "%s:\n", __func__);
>> +
>> +       if (state->mode != ADV7842_MODE_SDP)
>> +               return -ENODATA;
>> +
>> +       if (norm & V4L2_STD_ALL) {
>> +               state->norm = norm;
>> +               return 0;
>> +       }
>> +       return -EINVAL;
>> +}
> Why is there no hardware operation?
> 
> if (std == V4L2_STD_NTSC_443)
>                 val = 0x20;
> else if (std == V4L2_STD_PAL_60)
>                 val = 0x10;
> else if (std == V4L2_STD_PAL_Nc)
>                 val = 0x08;
> else if (std == V4L2_STD_PAL_M)
>                 val = 0x04;
> else if (std & V4L2_STD_NTSC)
>                 val = 0x02;
> else if (std & V4L2_STD_PAL)
>                 val = 0x01;
> else if (std & V4L2_STD_SECAM)
>                 val = 0x40;
> else
>                 return -EINVAL;
> /* force the digital core into a specific video standard */
> sdp_write(sd, 0x0, val);
> /* wait 100ms, otherwise color will be lost */
> msleep(100);
> state->std = std;
> return 0;

I checked with Martin Bugge who wrote this and the reason is that apparently forcing
the core to a specific standard will break the querystd functionality.

That said, this can definitely be improved by only forcing the standard when you start
streaming and reverting to the autodetect mode when streaming stops. QUERYSTD can return
-EBUSY when streaming is on.

I would prefer to do this in a separate patch later, though, as this will take some
time to implement and especially test.

Regards,

	Hans