Re: [PATCH 10/10] [media] tvp5150: Configure data interface via pdata or DT

From: Javier Martinez Canillas <javier@osg.samsung.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	Mauro Carvalho Chehab <mchehab@osg.samsung.com>,
	Enrico Butera <ebutera@gmail.com>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	Enric Balletbo i Serra <eballetbo@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	Eduard Gavin <egavinc@gmail.com>,
	Hans Verkuil <hans.verkuil@cisco.com>,
	linux-media@vger.kernel.org
Subject: Re: [PATCH 10/10] [media] tvp5150: Configure data interface via pdata or DT
Date: Wed, 6 Jan 2016 12:04:24 -0300	[thread overview]
Message-ID: <568D2CF8.9090802@osg.samsung.com> (raw)
In-Reply-To: <1895052.dqIgFQaCHk@avalon>

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