Hello Laurent,
On 01/06/2016 10:48 AM, Laurent Pinchart wrote:
[snip]
>>>> @@ -940,6 +948,16 @@ static int tvp5150_cropcap(struct v4l2_subdev *sd,
>>>> struct v4l2_cropcap *a)
>>>> static int tvp5150_g_mbus_config(struct v4l2_subdev *sd,
>>>> struct v4l2_mbus_config *cfg)
>>>> {
>>>> + struct tvp5150_platform_data *pdata = to_tvp5150(sd)->pdata;
>>>> +
>>>> + if (pdata) {
>>>> + cfg->type = pdata->bus_type;
>>>> + cfg->flags = pdata->parallel_flags;
>>>
>>> The clock and sync signals polarity don't seem configurable, shouldn't
>>> they just be hardcoded as currently done ?
>>
>> That's a very good question, I added the flags because according to
>> Documentation/devicetree/bindings/media/video-interfaces.txt, the way
>> to define that the output format will be BT.656 is to avoid defining
>> {hsync,vsync,field-even}-active properties.
>>
>> IOW, if parallel sync is used, then these properties have to be defined
>> and it felt strange to not use in the driver what is defined in the DT.
>
> In that case we should restrict the values of the properties to what the
> hardware actually supports. I would hardcode the flags here, and check them
> when parsing the endpoint to make sure they're valid.
>
That's a good idea, I'll also mention the supported values in the binding doc.
> If you find a register I have missed in the documentation with which
> polarities could be configured then please also feel free to prove me wrong
> :-)
>
I didn't find either when reading the datasheet to prepare this patch-set
so I think you are correct on that.
[snip]
>>>> +
>>>> + pdata->bus_type = bus_cfg.bus_type;
>>>> + pdata->parallel_flags = bus_cfg.bus.parallel.flags;
>>>
>>> The V4L2_MBUS_DATA_ACTIVE_HIGH flags set returned by
>>> tvp5150_g_mbus_config() when pdata is NULL is never set by
>>> v4l2_of_parse_endpoint(), should you add it unconditionally ?
>>
>> But v4l2_of_parse_endpoint() calls v4l2_of_parse_parallel_bus() which does
>> it or did I read the code incorrectly?
>
> No, you're right, I had overlooked the V4L2_MBUS_DATA_ACTIVE_HIGH flag when
> reading v4l2_of_parse_parallel_bus(), probably a typo when searching. Please
> ignore that comment.
>
Ok, thanks for the clarification.
Best regards,
--
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
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
