Hi Sylwester, Thanks for the review. On Sun, Jun 30, 2013 at 9:27 PM, Sylwester Nawrocki <sylvester.nawrocki@xxxxxxxxx> wrote: > Hi, > > > On 06/22/2013 07:44 PM, Prabhakar Lad wrote: >> >> From: "Lad, Prabhakar"<prabhakar.csengg@xxxxxxxxx> >> >> add OF support for the tvp7002 driver. >> >> Signed-off-by: Lad, Prabhakar<prabhakar.csengg@xxxxxxxxx> >> Cc: Hans Verkuil<hans.verkuil@xxxxxxxxx> >> Cc: Laurent Pinchart<laurent.pinchart@xxxxxxxxxxxxxxxx> >> Cc: Mauro Carvalho Chehab<mchehab@xxxxxxxxxx> >> Cc: Guennadi Liakhovetski<g.liakhovetski@xxxxxx> >> Cc: Sylwester Nawrocki<s.nawrocki@xxxxxxxxxxx> >> Cc: Sakari Ailus<sakari.ailus@xxxxxx> >> Cc: Grant Likely<grant.likely@xxxxxxxxxxxx> >> Cc: Rob Herring<rob.herring@xxxxxxxxxxx> >> Cc: Rob Landley<rob@xxxxxxxxxxx> >> Cc: devicetree-discuss@xxxxxxxxxxxxxxxx >> Cc: linux-doc@xxxxxxxxxxxxxxx >> Cc: linux-kernel@xxxxxxxxxxxxxxx >> Cc: davinci-linux-open-source@xxxxxxxxxxxxxxxxxxxx Will take care of this as mentioned in earlier patch. >> --- >> Depends on patch https://patchwork.kernel.org/patch/2765851/ >> >> .../devicetree/bindings/media/i2c/tvp7002.txt | 43 +++++++++++++ >> drivers/media/i2c/tvp7002.c | 67 >> ++++++++++++++++++-- >> 2 files changed, 103 insertions(+), 7 deletions(-) >> create mode 100644 >> Documentation/devicetree/bindings/media/i2c/tvp7002.txt >> >> diff --git a/Documentation/devicetree/bindings/media/i2c/tvp7002.txt >> b/Documentation/devicetree/bindings/media/i2c/tvp7002.txt >> new file mode 100644 >> index 0000000..9daebe1 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/media/i2c/tvp7002.txt >> @@ -0,0 +1,43 @@ >> +* Texas Instruments TV7002 video decoder >> + >> +The TVP7002 device supports digitizing of video and graphics signal in >> RGB and >> +YPbPr color space. >> + >> +Required Properties : >> +- compatible : Must be "ti,tvp7002" >> + >> +- hsync-active: HSYNC Polarity configuration for endpoint. >> + >> +- vsync-active: VSYNC Polarity configuration for endpoint. >> + >> +- pclk-sample: Clock polarity of the endpoint. >> + >> +- video-sync: Video sync property of the endpoint. >> + >> +- ti,tvp7002-fid-polarity: Active-high Field ID polarity of the endpoint. > > > I thought it was agreed 'field-even-active' would be used instead of > this device specific property. Did you run into any issues with that ? > > Argh I some how missed it out, sorry this should be 'field-even-active' >> + >> +For further reading of port node refer >> Documentation/devicetree/bindings/media/ >> +video-interfaces.txt. >> + >> +Example: >> + >> + i2c0@1c22000 { >> + ... >> + ... >> + tvp7002@5c { >> + compatible = "ti,tvp7002"; >> + reg =<0x5c>; >> + >> + port { >> + tvp7002_1: endpoint { >> + hsync-active =<1>; >> + vsync-active =<1>; >> + pclk-sample =<0>; >> + video-sync >> =<V4L2_MBUS_VIDEO_SYNC_ON_GREEN>; >> + ti,tvp7002-fid-polarity; >> + }; >> + }; >> + }; >> + ... >> + }; >> diff --git a/drivers/media/i2c/tvp7002.c b/drivers/media/i2c/tvp7002.c >> index b577548..4896024 100644 >> --- a/drivers/media/i2c/tvp7002.c >> +++ b/drivers/media/i2c/tvp7002.c >> @@ -35,6 +35,8 @@ >> #include<media/v4l2-device.h> >> #include<media/v4l2-common.h> >> #include<media/v4l2-ctrls.h> >> +#include<media/v4l2-of.h> >> + >> #include "tvp7002_reg.h" >> >> MODULE_DESCRIPTION("TI TVP7002 Video and Graphics Digitizer driver"); >> @@ -943,6 +945,48 @@ static const struct v4l2_subdev_ops tvp7002_ops = { >> .pad =&tvp7002_pad_ops, >> }; >> >> +static struct tvp7002_config * >> +tvp7002_get_pdata(struct i2c_client *client) > > > nit: unnecessary line break > Ok >> +{ >> + struct v4l2_of_endpoint bus_cfg; >> + struct tvp7002_config *pdata; >> + struct device_node *endpoint; >> + unsigned int flags; >> + >> + if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node) >> + return client->dev.platform_data; >> + >> + endpoint = v4l2_of_get_next_endpoint(client->dev.of_node, NULL); >> + if (!endpoint) >> + return NULL; >> + >> + pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL); >> + if (!pdata) >> + goto done; >> + >> + v4l2_of_parse_endpoint(endpoint,&bus_cfg); >> + flags = bus_cfg.bus.parallel.flags; >> + >> + if (flags& V4L2_MBUS_HSYNC_ACTIVE_HIGH) >> >> + pdata->hs_polarity = 1; >> + >> + if (flags& V4L2_MBUS_VSYNC_ACTIVE_HIGH) >> >> + pdata->vs_polarity = 1; >> + >> + if (flags& V4L2_MBUS_PCLK_SAMPLE_RISING) >> >> + pdata->clk_polarity = 1; >> + >> + if (flags& V4L2_MBUS_VIDEO_SYNC_ON_GREEN) >> >> + pdata->sog_polarity = 1; > > > This clearly shows that you're using this property for something different > you have defined it for. I asked previously if what you really needed for > this TVP7002 chip is a DT property indicating sync-on-green _polarity_ or > the sync-on-green _usage_. And you clearly need the polarity information. > > So I would just define a standard "sync-on-green-active" property and > V4L2_MBUS_VIDEO_SOG_ACTIVE_{HIGH,LOW} flags. Presumably you don't need > anything else, and the extra sync polarity flags would need eventually > to be defined anyway, independently of any video sync selection property, > should something like this ever need to be specified by firmware. > > OK >> + pdata->fid_polarity = of_property_read_bool(endpoint, >> + >> "ti,tvp7002-fid-polarity"); > > > This could be just: > > if (flags & V4L2_MBUS_FIELD_EVEN_HIGH) > pdata->fid_polarity = 1; > > if you used standard 'field-even-active' property. > Agreed. > And this is what we find in the TVP7002 datasheet, in the section describing > MISC Control 3 (18h) register's FID POL bit (pdata->fid_polarity is written > directly to FID POL): > > "FID POL: Active-high Field ID output polarity control. Under normal > operation, the field ID output is set to logic 1 for an odd field (field 1) > and set to logic 0 for an even field (field 0). > 0 = Normal operation (default) > 1 = FID output polarity inverted > NOTE: This control bit also affects the polarity of the data enable output > when selected (see Test output control [2:0] at subaddress 17h)." > > > And include/media/tvp70002.h: > > * fid_polarity: > * 0 -> the field ID output is set to logic 1 for an > odd > * field (field 1) and set to logic 0 for an even > * field (field 0). > * 1 -> operation with polarity inverted. > > > Do you know if the chip automatically selects video sync source > (sync-on-green > vs. VSYNC/HSYNC) and there is no need to configure this on the analogue > input > side ? At least the driver seems to always select the default SOGIN_1 input > (TVP7002_IN_MUX_SEL_1 register is set only at initialization time). > Yes the driver is selecting the default SOGIN_1 input. > Or perhaps it just outputs on SOGOUT, VSOUT, HSOUT lines whatever is fed to > its analogue inputs, and any further processing unit need to determine what > synchronization signal is present and should be used ? > Yes that correct, there is a register (Sync Detect Status) which detects the sync for you. > I suspect that we don't need, e.g. another endpoint node to specify the > configuration of the TVP7002 analogue input interface, that would contain > a property like video-sync. > > If I understand correctly you mean if there are two tvp7002 devices connected we don’t need to specify video-sync property, but my question how do we specify this property in common then ? Regards, --Prabhakar Lad -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
