public inbox for linux-media@vger.kernel.org
 help / color / mirror / Atom feed
* Few Doubts on adding DT nodes for bridge driver
@ 2013-07-21  6:20 Prabhakar Lad
  2013-07-21  7:42 ` Guennadi Liakhovetski
  2013-07-21  9:24 ` Sylwester Nawrocki
  0 siblings, 2 replies; 6+ messages in thread
From: Prabhakar Lad @ 2013-07-21  6:20 UTC (permalink / raw)
  To: Sylwester Nawrocki, Guennadi Liakhovetski; +Cc: linux-media

Hi Sylwester, Guennadi,

I am working on adding DT support for VPIF driver, initially to get
some hands dirty
on working on Capture driver and later will move ahead to add for the display.
I have added asynchronous probing support for the both the bridge and subdevs
which works perfectly like on a charm with passing pdata as usually,
but doing the
same with DT I have few doubts on building the pdata in the bridge driver.


This is a snippet of my subdes in i2c node:-

i2c0: i2c@1c22000 {
			status = "okay";
			clock-frequency = <100000>;
			pinctrl-names = "default";
			pinctrl-0 = <&i2c0_pins>;

			tvp514x@5c {
				compatible = "ti,tvp5146";
				reg = <0x5c>;

				port {
					tvp514x_1: endpoint {
						remote-endpoint = <&vpif_capture0_1>;
						hsync-active = <1>;
						vsync-active = <1>;
						pclk-sample = <0>;
					};
				};
			};

			tvp514x@5d {
				compatible = "ti,tvp5146";
				reg = <0x5d>;

				port {
					tvp514x_2: endpoint {
						remote-endpoint = <&vpif_capture0_0>;
						hsync-active = <1>;
						vsync-active = <1>;
						pclk-sample = <0>;
					};
				};
			};
                 ......
		};

Here tvp514x are the subdevs the platform has two of them one at 0x5c and 0x5d,
so I have added two nodes for them.

Following is DT node for the bridge driver:-

	vpif_capture@0 {
		status = "okay";
		port {
			vpif_capture0_1: endpoint@1 {
				remote = <&tvp514x_1>;
			};
			vpif_capture0_0: endpoint@0 {
				remote = <&tvp514x_2>;
			};
		};
	};
I have added two endpoints for the bridge driver. In the bridge driver
to build the pdata from DT node,I do the following,

np = v4l2_of_get_next_endpoint(pdev->dev.of_node, NULL);
The above will give the first endpoint ie, endpoint@1
>From here is it possible to get the tvp514x_1 endpoint node and the
parent of it?
so that I  build the asynchronous subdev list for the bridge driver.


+static struct v4l2_async_subdev tvp1_sd = {
+       .hw = {
+               .bus_type = V4L2_ASYNC_BUS_I2C,
+               .match.i2c = {
+                       .adapter_id = 1,
+                       .address = 0x5c,
+               },
+       },
+};

For building the asd subdev list in the bridge driver I can get the
address easily,
how do I get the adapter_id ? should this be a property subdev ? And also same
with bustype.

Regards,
--Prabhakar

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Few Doubts on adding DT nodes for bridge driver
  2013-07-21  6:20 Few Doubts on adding DT nodes for bridge driver Prabhakar Lad
@ 2013-07-21  7:42 ` Guennadi Liakhovetski
  2013-07-21  9:24 ` Sylwester Nawrocki
  1 sibling, 0 replies; 6+ messages in thread
From: Guennadi Liakhovetski @ 2013-07-21  7:42 UTC (permalink / raw)
  To: Prabhakar Lad; +Cc: Sylwester Nawrocki, linux-media

Hi Prabhakar

On Sun, 21 Jul 2013, Prabhakar Lad wrote:

> Hi Sylwester, Guennadi,
> 
> I am working on adding DT support for VPIF driver, initially to get
> some hands dirty
> on working on Capture driver and later will move ahead to add for the display.
> I have added asynchronous probing support for the both the bridge and subdevs
> which works perfectly like on a charm with passing pdata as usually,
> but doing the
> same with DT I have few doubts on building the pdata in the bridge driver.
> 
> 
> This is a snippet of my subdes in i2c node:-
> 
> i2c0: i2c@1c22000 {
> 			status = "okay";
> 			clock-frequency = <100000>;
> 			pinctrl-names = "default";
> 			pinctrl-0 = <&i2c0_pins>;
> 
> 			tvp514x@5c {
> 				compatible = "ti,tvp5146";
> 				reg = <0x5c>;
> 
> 				port {
> 					tvp514x_1: endpoint {
> 						remote-endpoint = <&vpif_capture0_1>;
> 						hsync-active = <1>;
> 						vsync-active = <1>;
> 						pclk-sample = <0>;
> 					};
> 				};
> 			};
> 
> 			tvp514x@5d {
> 				compatible = "ti,tvp5146";
> 				reg = <0x5d>;
> 
> 				port {
> 					tvp514x_2: endpoint {
> 						remote-endpoint = <&vpif_capture0_0>;
> 						hsync-active = <1>;
> 						vsync-active = <1>;
> 						pclk-sample = <0>;
> 					};
> 				};
> 			};
>                  ......
> 		};
> 
> Here tvp514x are the subdevs the platform has two of them one at 0x5c and 0x5d,
> so I have added two nodes for them.
> 
> Following is DT node for the bridge driver:-
> 
> 	vpif_capture@0 {
> 		status = "okay";
> 		port {
> 			vpif_capture0_1: endpoint@1 {
> 				remote = <&tvp514x_1>;
> 			};
> 			vpif_capture0_0: endpoint@0 {
> 				remote = <&tvp514x_2>;
> 			};
> 		};
> 	};
> I have added two endpoints for the bridge driver. In the bridge driver
> to build the pdata from DT node,I do the following,
> 
> np = v4l2_of_get_next_endpoint(pdev->dev.of_node, NULL);
> The above will give the first endpoint ie, endpoint@1
> >From here is it possible to get the tvp514x_1 endpoint node and the
> parent of it?

Yes, you can use something like of_parse_phandle().

Thanks
Guennadi

> so that I  build the asynchronous subdev list for the bridge driver.
> 
> 
> +static struct v4l2_async_subdev tvp1_sd = {
> +       .hw = {
> +               .bus_type = V4L2_ASYNC_BUS_I2C,
> +               .match.i2c = {
> +                       .adapter_id = 1,
> +                       .address = 0x5c,
> +               },
> +       },
> +};
> 
> For building the asd subdev list in the bridge driver I can get the
> address easily,
> how do I get the adapter_id ? should this be a property subdev ? And also same
> with bustype.
> 
> Regards,
> --Prabhakar
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Few Doubts on adding DT nodes for bridge driver
  2013-07-21  6:20 Few Doubts on adding DT nodes for bridge driver Prabhakar Lad
  2013-07-21  7:42 ` Guennadi Liakhovetski
@ 2013-07-21  9:24 ` Sylwester Nawrocki
  2013-07-21  9:47   ` Guennadi Liakhovetski
  2013-07-21 12:05   ` Prabhakar Lad
  1 sibling, 2 replies; 6+ messages in thread
From: Sylwester Nawrocki @ 2013-07-21  9:24 UTC (permalink / raw)
  To: Prabhakar Lad; +Cc: Guennadi Liakhovetski, linux-media

Hi Prabhakar,

On 07/21/2013 08:20 AM, Prabhakar Lad wrote:
> Hi Sylwester, Guennadi,
>
> I am working on adding DT support for VPIF driver, initially to get
> some hands dirty
> on working on Capture driver and later will move ahead to add for the display.
> I have added asynchronous probing support for the both the bridge and subdevs
> which works perfectly like on a charm with passing pdata as usually,
> but doing the
> same with DT I have few doubts on building the pdata in the bridge driver.
>
>
> This is a snippet of my subdes in i2c node:-
>
> i2c0: i2c@1c22000 {
> 			status = "okay";
> 			clock-frequency =<100000>;
> 			pinctrl-names = "default";
> 			pinctrl-0 =<&i2c0_pins>;
>
> 			tvp514x@5c {
> 				compatible = "ti,tvp5146";
> 				reg =<0x5c>;
>
> 				port {
> 					tvp514x_1: endpoint {
> 						remote-endpoint =<&vpif_capture0_1>;
> 						hsync-active =<1>;
> 						vsync-active =<1>;
> 						pclk-sample =<0>;
> 					};
> 				};
> 			};
>
> 			tvp514x@5d {
> 				compatible = "ti,tvp5146";
> 				reg =<0x5d>;
>
> 				port {
> 					tvp514x_2: endpoint {
> 						remote-endpoint =<&vpif_capture0_0>;
> 						hsync-active =<1>;
> 						vsync-active =<1>;
> 						pclk-sample =<0>;
> 					};
> 				};
> 			};
>                   ......
> 		};
>
> Here tvp514x are the subdevs the platform has two of them one at 0x5c and 0x5d,
> so I have added two nodes for them.
>
> Following is DT node for the bridge driver:-
>
> 	vpif_capture@0 {
> 		status = "okay";
> 		port {

You should also have:
			#address-cells = <1>;
			#size-cells = <0>;

here or in vpif_capture node.

> 			vpif_capture0_1: endpoint@1 {
> 				remote =<&tvp514x_1>;
> 			};
> 			vpif_capture0_0: endpoint@0 {
> 				remote =<&tvp514x_2>;
> 			};
> 		};
> 	};

Are tvp514x@5c and tvp514x@5d decoders really connected to same bus, or are
they on separate busses ? If the latter then you should have 2 'port' 
nodes.
And in such case don't you need to identify to which

> I have added two endpoints for the bridge driver. In the bridge driver
> to build the pdata from DT node,I do the following,
>
> np = v4l2_of_get_next_endpoint(pdev->dev.of_node, NULL);
>
> The above will give the first endpoint ie, endpoint@1
>  From here is it possible to get the tvp514x_1 endpoint node and the
> parent of it?

Isn't v4l2_of_get_remote_port_parent() what you need ?

> so that I  build the asynchronous subdev list for the bridge driver.
>
>
> +static struct v4l2_async_subdev tvp1_sd = {
> +       .hw = {

This doesn't match the current struct v4l2_async_subdev data strcucture,
there is no 'hw' field now.

> +               .bus_type = V4L2_ASYNC_BUS_I2C,
> +               .match.i2c = {
> +                       .adapter_id = 1,
> +                       .address = 0x5c,
> +               },
> +       },
> +};
>
> For building the asd subdev list in the bridge driver I can get the
> address easily,
> how do I get the adapter_id ? should this be a property subdev ? And also same
> with bustype.

I had been working on the async subdev registration support in the 
exynos4-is
driver this week and I have a few patches for v4l2-async.
What those patches do is renaming V4L2_ASYNC_BUS_* to V4L2_ASYNC_MATCH_*,
adding V4L2_ASYNC_MATCH_OF and a corresponding match_of callback like:

static bool match_of(struct device *dev, struct v4l2_async_subdev *asd)
{
	return dev->of_node == asd->match.of.node;
}

Then a driver registering the notifier, after parsing the device tree, can
just pass a list of DT node pointers corresponding to its subdevs.

All this could also be achieved with V4L2_ASYNC_BUS_CUSTOM, but I think it's
better to make it as simple as possible for drivers by extending the core
a little.

I'm going to post those patches as RFC on Monday.

--
Regards,
Sylwester

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Few Doubts on adding DT nodes for bridge driver
  2013-07-21  9:24 ` Sylwester Nawrocki
@ 2013-07-21  9:47   ` Guennadi Liakhovetski
  2013-07-21 13:12     ` Prabhakar Lad
  2013-07-21 12:05   ` Prabhakar Lad
  1 sibling, 1 reply; 6+ messages in thread
From: Guennadi Liakhovetski @ 2013-07-21  9:47 UTC (permalink / raw)
  To: Sylwester Nawrocki; +Cc: Prabhakar Lad, linux-media

On Sun, 21 Jul 2013, Sylwester Nawrocki wrote:

> Hi Prabhakar,
> 
> On 07/21/2013 08:20 AM, Prabhakar Lad wrote:
> > Hi Sylwester, Guennadi,
> > 
> > I am working on adding DT support for VPIF driver, initially to get
> > some hands dirty
> > on working on Capture driver and later will move ahead to add for the
> > display.
> > I have added asynchronous probing support for the both the bridge and
> > subdevs
> > which works perfectly like on a charm with passing pdata as usually,
> > but doing the
> > same with DT I have few doubts on building the pdata in the bridge driver.
> > 
> > 
> > This is a snippet of my subdes in i2c node:-
> > 
> > i2c0: i2c@1c22000 {
> > 			status = "okay";
> > 			clock-frequency =<100000>;
> > 			pinctrl-names = "default";
> > 			pinctrl-0 =<&i2c0_pins>;
> > 
> > 			tvp514x@5c {
> > 				compatible = "ti,tvp5146";
> > 				reg =<0x5c>;
> > 
> > 				port {
> > 					tvp514x_1: endpoint {
> > 						remote-endpoint
> > =<&vpif_capture0_1>;
> > 						hsync-active =<1>;
> > 						vsync-active =<1>;
> > 						pclk-sample =<0>;
> > 					};
> > 				};
> > 			};
> > 
> > 			tvp514x@5d {
> > 				compatible = "ti,tvp5146";
> > 				reg =<0x5d>;
> > 
> > 				port {
> > 					tvp514x_2: endpoint {
> > 						remote-endpoint
> > =<&vpif_capture0_0>;
> > 						hsync-active =<1>;
> > 						vsync-active =<1>;
> > 						pclk-sample =<0>;
> > 					};
> > 				};
> > 			};
> >                   ......
> > 		};
> > 
> > Here tvp514x are the subdevs the platform has two of them one at 0x5c and
> > 0x5d,
> > so I have added two nodes for them.
> > 
> > Following is DT node for the bridge driver:-
> > 
> > 	vpif_capture@0 {
> > 		status = "okay";
> > 		port {
> 
> You should also have:
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 
> here or in vpif_capture node.
> 
> > 			vpif_capture0_1: endpoint@1 {
> > 				remote =<&tvp514x_1>;
> > 			};
> > 			vpif_capture0_0: endpoint@0 {
> > 				remote =<&tvp514x_2>;

BTW, just occurred to me: shouldn't also these rather be 
"remote-endpoint?" The documentation example should then be fixed too.

> > 			};
> > 		};
> > 	};
> 
> Are tvp514x@5c and tvp514x@5d decoders really connected to same bus, or are
> they on separate busses ? If the latter then you should have 2 'port' nodes.
> And in such case don't you need to identify to which
> 
> > I have added two endpoints for the bridge driver. In the bridge driver
> > to build the pdata from DT node,I do the following,
> > 
> > np = v4l2_of_get_next_endpoint(pdev->dev.of_node, NULL);
> > 
> > The above will give the first endpoint ie, endpoint@1
> >  From here is it possible to get the tvp514x_1 endpoint node and the
> > parent of it?
> 
> Isn't v4l2_of_get_remote_port_parent() what you need ?

Right, forgot we've got a helper for that already.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Few Doubts on adding DT nodes for bridge driver
  2013-07-21  9:24 ` Sylwester Nawrocki
  2013-07-21  9:47   ` Guennadi Liakhovetski
@ 2013-07-21 12:05   ` Prabhakar Lad
  1 sibling, 0 replies; 6+ messages in thread
From: Prabhakar Lad @ 2013-07-21 12:05 UTC (permalink / raw)
  To: Sylwester Nawrocki; +Cc: Guennadi Liakhovetski, linux-media

Hi Sylwester,

On Sun, Jul 21, 2013 at 2:54 PM, Sylwester Nawrocki
<sylvester.nawrocki@gmail.com> wrote:
> Hi Prabhakar,
>
>
> On 07/21/2013 08:20 AM, Prabhakar Lad wrote:
>>
>> Hi Sylwester, Guennadi,
>>
>> I am working on adding DT support for VPIF driver, initially to get
>> some hands dirty
>> on working on Capture driver and later will move ahead to add for the
>> display.
>> I have added asynchronous probing support for the both the bridge and
>> subdevs
>> which works perfectly like on a charm with passing pdata as usually,
>> but doing the
>> same with DT I have few doubts on building the pdata in the bridge driver.
>>
>>
>> This is a snippet of my subdes in i2c node:-
>>
>> i2c0: i2c@1c22000 {
>>                         status = "okay";
>>                         clock-frequency =<100000>;
>>                         pinctrl-names = "default";
>>                         pinctrl-0 =<&i2c0_pins>;
>>
>>                         tvp514x@5c {
>>                                 compatible = "ti,tvp5146";
>>                                 reg =<0x5c>;
>>
>>                                 port {
>>                                         tvp514x_1: endpoint {
>>                                                 remote-endpoint
>> =<&vpif_capture0_1>;
>>                                                 hsync-active =<1>;
>>                                                 vsync-active =<1>;
>>                                                 pclk-sample =<0>;
>>                                         };
>>                                 };
>>                         };
>>
>>                         tvp514x@5d {
>>                                 compatible = "ti,tvp5146";
>>                                 reg =<0x5d>;
>>
>>                                 port {
>>                                         tvp514x_2: endpoint {
>>                                                 remote-endpoint
>> =<&vpif_capture0_0>;
>>                                                 hsync-active =<1>;
>>                                                 vsync-active =<1>;
>>                                                 pclk-sample =<0>;
>>                                         };
>>                                 };
>>                         };
>>                   ......
>>                 };
>>
>> Here tvp514x are the subdevs the platform has two of them one at 0x5c and
>> 0x5d,
>> so I have added two nodes for them.
>>
>> Following is DT node for the bridge driver:-
>>
>>         vpif_capture@0 {
>>                 status = "okay";
>>                 port {
>
>
> You should also have:
>                         #address-cells = <1>;
>                         #size-cells = <0>;
>
> here or in vpif_capture node.
>
Ok

>
>>                         vpif_capture0_1: endpoint@1 {
>>                                 remote =<&tvp514x_1>;
>>                         };
>>                         vpif_capture0_0: endpoint@0 {
>>                                 remote =<&tvp514x_2>;
>>                         };
>>                 };
>>         };
>
>
> Are tvp514x@5c and tvp514x@5d decoders really connected to same bus, or are
> they on separate busses ? If the latter then you should have 2 'port' nodes.
> And in such case don't you need to identify to which
>
The tvp514x@5c and tvp514x@5d are connected to the same bus.

>
>> I have added two endpoints for the bridge driver. In the bridge driver
>> to build the pdata from DT node,I do the following,
>>
>> np = v4l2_of_get_next_endpoint(pdev->dev.of_node, NULL);
>>
>> The above will give the first endpoint ie, endpoint@1
>>  From here is it possible to get the tvp514x_1 endpoint node and the
>> parent of it?
>
>
> Isn't v4l2_of_get_remote_port_parent() what you need ?
>
Al rite I'll check on it.

>
>> so that I  build the asynchronous subdev list for the bridge driver.
>>
>>
>> +static struct v4l2_async_subdev tvp1_sd = {
>> +       .hw = {
>
>
> This doesn't match the current struct v4l2_async_subdev data strcucture,
> there is no 'hw' field now.
>
>
Ah my bad pasted a wrong one earlier one :)

>> +               .bus_type = V4L2_ASYNC_BUS_I2C,
>> +               .match.i2c = {
>> +                       .adapter_id = 1,
>> +                       .address = 0x5c,
>> +               },
>> +       },
>> +};
>>
>> For building the asd subdev list in the bridge driver I can get the
>> address easily,
>> how do I get the adapter_id ? should this be a property subdev ? And also
>> same
>> with bustype.
>
>
> I had been working on the async subdev registration support in the
> exynos4-is
> driver this week and I have a few patches for v4l2-async.
> What those patches do is renaming V4L2_ASYNC_BUS_* to V4L2_ASYNC_MATCH_*,
> adding V4L2_ASYNC_MATCH_OF and a corresponding match_of callback like:
>
> static bool match_of(struct device *dev, struct v4l2_async_subdev *asd)
> {
>         return dev->of_node == asd->match.of.node;
> }
>
> Then a driver registering the notifier, after parsing the device tree, can
> just pass a list of DT node pointers corresponding to its subdevs.
>
> All this could also be achieved with V4L2_ASYNC_BUS_CUSTOM, but I think it's
> better to make it as simple as possible for drivers by extending the core
> a little.
>
> I'm going to post those patches as RFC on Monday.
>
That's cool, It will be very useful for me to then I'll be waiting for
it then :-)

Regards,
--Prabhakar Lad

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Few Doubts on adding DT nodes for bridge driver
  2013-07-21  9:47   ` Guennadi Liakhovetski
@ 2013-07-21 13:12     ` Prabhakar Lad
  0 siblings, 0 replies; 6+ messages in thread
From: Prabhakar Lad @ 2013-07-21 13:12 UTC (permalink / raw)
  To: Guennadi Liakhovetski; +Cc: Sylwester Nawrocki, linux-media

Hi Guennadi,

On Sun, Jul 21, 2013 at 3:17 PM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> On Sun, 21 Jul 2013, Sylwester Nawrocki wrote:
>
[snip]
>> >                             remote =<&tvp514x_2>;
>
> BTW, just occurred to me: shouldn't also these rather be
> "remote-endpoint?" The documentation example should then be fixed too.
>
Ah correct I was referring the same and added in the 'remote' in device node.
Shall I go ahead and post a patch fixing it ?

>> >                     };
>> >             };
>> >     };
>>
>> Are tvp514x@5c and tvp514x@5d decoders really connected to same bus, or are
>> they on separate busses ? If the latter then you should have 2 'port' nodes.
>> And in such case don't you need to identify to which
>>
>> > I have added two endpoints for the bridge driver. In the bridge driver
>> > to build the pdata from DT node,I do the following,
>> >
>> > np = v4l2_of_get_next_endpoint(pdev->dev.of_node, NULL);
>> >
>> > The above will give the first endpoint ie, endpoint@1
>> >  From here is it possible to get the tvp514x_1 endpoint node and the
>> > parent of it?
>>
>> Isn't v4l2_of_get_remote_port_parent() what you need ?
>
> Right, forgot we've got a helper for that already.
>
Yes this works for me now thank :)

Regards,
--Prabhakar Lad

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2013-07-21 13:12 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-07-21  6:20 Few Doubts on adding DT nodes for bridge driver Prabhakar Lad
2013-07-21  7:42 ` Guennadi Liakhovetski
2013-07-21  9:24 ` Sylwester Nawrocki
2013-07-21  9:47   ` Guennadi Liakhovetski
2013-07-21 13:12     ` Prabhakar Lad
2013-07-21 12:05   ` Prabhakar Lad

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox