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 devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html