Messages in this thread | | | Date | Sun, 30 Jun 2013 17:57:28 +0200 | From | Sylwester Nawrocki <> | Subject | Re: [PATCH v2 2/2] media: i2c: tvp7002: add OF support |
| |
Hi,
On 06/22/2013 07:44 PM, Prabhakar Lad wrote: > From: "Lad, Prabhakar"<prabhakar.csengg@gmail.com> > > add OF support for the tvp7002 driver. > > Signed-off-by: Lad, Prabhakar<prabhakar.csengg@gmail.com> > Cc: Hans Verkuil<hans.verkuil@cisco.com> > Cc: Laurent Pinchart<laurent.pinchart@ideasonboard.com> > Cc: Mauro Carvalho Chehab<mchehab@redhat.com> > Cc: Guennadi Liakhovetski<g.liakhovetski@gmx.de> > Cc: Sylwester Nawrocki<s.nawrocki@samsung.com> > Cc: Sakari Ailus<sakari.ailus@iki.fi> > Cc: Grant Likely<grant.likely@secretlab.ca> > Cc: Rob Herring<rob.herring@calxeda.com> > Cc: Rob Landley<rob@landley.net> > Cc: devicetree-discuss@lists.ozlabs.org > Cc: linux-doc@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: davinci-linux-open-source@linux.davincidsp.com > --- > 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 ?
> + > +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
> +{ > + 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.
> + 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.
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).
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 ?
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.
> +done: > + of_node_put(endpoint); > + return pdata; > +}
Regards, Sylwester
| |