linux-kernel - Re: [PATCH v6 2/3] media: i2c: adv748x: add adv748x driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <43cf18ed-26ee-384d-192b-9c0c672c0c90@xs4all.nl>
Date:   Mon, 3 Jul 2017 16:54:52 +0200
From:   Hans Verkuil <hverkuil@...all.nl>
To:     kieran.bingham@...asonboard.com, linux-renesas-soc@...r.kernel.org,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     laurent.pinchart@...asonboard.com, niklas.soderlund@...natech.se,
        hans.verkuil@...co.com,
        Kieran Bingham <kieran.bingham+renesas@...asonboard.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Steve Longerbeam <slongerbeam@...il.com>,
        Pavel Machek <pavel@....cz>,
        Ramesh Shanmugasundaram <ramesh.shanmugasundaram@...renesas.com>,
        Todor Tomov <todor.tomov@...aro.org>,
        Hyungwoo Yang <hyungwoo.yang@...el.com>
Subject: Re: [PATCH v6 2/3] media: i2c: adv748x: add adv748x driver

On 07/03/2017 04:45 PM, Kieran Bingham wrote:
>>> +#define ADV748X_HDMI_MAX_PIXELCLOCK    162000000
>>
>> You probably based that on the 1600x1200p60 format?
> 
> No idea I'm afraid - it's the value that was set when I recieved the driver...
> 
>>
>> 162MHz is a bit low for an adv receiver. The adv7604 and adv8742 have a max rate
>> of 225 MHz.
>> This should be documented in the datasheet.
> 
> Hrm ... haven't found it yet - I'll keep digging....

This is often documented in the hardware datasheet, not the programming datasheet.

>>> +static void adv748x_hdmi_fill_format(struct adv748x_hdmi *hdmi,
>>> +                     struct v4l2_mbus_framefmt *fmt)
>>> +{
>>> +    memset(fmt, 0, sizeof(*fmt));
>>> +
>>> +    fmt->code = MEDIA_BUS_FMT_RGB888_1X24;
>>> +    fmt->field = hdmi->timings.bt.interlaced ?
>>> +        V4L2_FIELD_INTERLACED : V4L2_FIELD_NONE;
>>
>> INTERLACED -> ALTERNATE.
> 
> OK.
> 
> OOI: Is this because of the removal of the interlaced cap - or because
> V4L2_FIELD_INTERLACED is deprecated or such?

Unless the adv748x has a deinterlacer you will receive the fields as separate
transfers.

FIELD_INTERLACED means that the two fields are combined into a single frame, but
I doubt that that's what happens. FIELD_ALTERNATE means that the top and bottom
fields are send one after the other.

FIELD_INTERLACED is very common for SDTV, but FIELD_ALTERNATE is almost always
used for HDTV.

That reminds me: in adv748x_afe_fill_format you also set FIELD_INTERLACED, but
is that correct? Shouldn't that also be FIELD_ALTERNATE?

> 
>>> +
>>> +    /* The colorspace depends on the AVI InfoFrame contents */

Missed that in the first review: add a 'TODO:' before this comment.

>>> +    fmt->colorspace = V4L2_COLORSPACE_SRGB;
>>> +
>>> +    fmt->width = hdmi->timings.bt.width;
>>> +    fmt->height = hdmi->timings.bt.height;
>>> +}

Regards,

	Hans