| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CA+V-a8sW+D8trces5AXu__Lw9F7TO6fCcQW+LGZKRhA41uOEfw@mail.gmail.com>
Date: Thu, 11 Jul 2013 22:39:10 +0530
From: Prabhakar Lad <prabhakar.csengg@...il.com>
To: Sylwester Nawrocki <sylvester.nawrocki@...il.com>
Cc: Mauro Carvalho Chehab <mchehab@...hat.com>,
LMML <linux-media@...r.kernel.org>,
Hans Verkuil <hans.verkuil@...co.com>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
DLOS <davinci-linux-open-source@...ux.davincidsp.com>,
LKML <linux-kernel@...r.kernel.org>,
Guennadi Liakhovetski <g.liakhovetski@....de>,
Sylwester Nawrocki <s.nawrocki@...sung.com>,
Sakari Ailus <sakari.ailus@....fi>,
Grant Likely <grant.likely@...retlab.ca>,
Rob Herring <rob.herring@...xeda.com>,
Rob Landley <rob@...dley.net>,
devicetree-discuss@...ts.ozlabs.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH v2 2/2] media: i2c: tvp7002: add OF support
Hi Sylwester,
Thanks for the review.
On Sun, Jun 30, 2013 at 9:27 PM, Sylwester Nawrocki
<sylvester.nawrocki@...il.com> wrote:
> Hi,
>
>
> On 06/22/2013 07:44 PM, Prabhakar Lad wrote:
>>
>> From: "Lad, Prabhakar"<prabhakar.csengg@...il.com>
>>
>> add OF support for the tvp7002 driver.
>>
>> Signed-off-by: Lad, Prabhakar<prabhakar.csengg@...il.com>
>> Cc: Hans Verkuil<hans.verkuil@...co.com>
>> Cc: Laurent Pinchart<laurent.pinchart@...asonboard.com>
>> Cc: Mauro Carvalho Chehab<mchehab@...hat.com>
>> Cc: Guennadi Liakhovetski<g.liakhovetski@....de>
>> Cc: Sylwester Nawrocki<s.nawrocki@...sung.com>
>> Cc: Sakari Ailus<sakari.ailus@....fi>
>> Cc: Grant Likely<grant.likely@...retlab.ca>
>> Cc: Rob Herring<rob.herring@...xeda.com>
>> Cc: Rob Landley<rob@...dley.net>
>> Cc: devicetree-discuss@...ts.ozlabs.org
>> Cc: linux-doc@...r.kernel.org
>> Cc: linux-kernel@...r.kernel.org
>> Cc: davinci-linux-open-source@...ux.davincidsp.com
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@...2000 {
>> + ...
>> + ...
>> + 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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists