Re: [PATCH v14 20/28] v4l: fwnode: Add a helper function to obtain device / integer references

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Oct 16, 2017 at 04:02:45PM +0300, Sakari Ailus wrote:
> Hi Hans,
> 
> On Tue, Oct 10, 2017 at 03:07:29PM +0200, Hans Verkuil wrote:
> > On 10/10/2017 01:27 PM, Sakari Ailus wrote:
> > > Hi Hans,
> > > 
> > > On Mon, Oct 09, 2017 at 02:06:55PM +0200, Hans Verkuil wrote:
> > >> Hi Sakari,
> > >>
> > >> My reply here is also valid for v15.
> > >>
> > >> On 26/09/17 13:30, Sakari Ailus wrote:
> > >>> Hi Hans,
> > >>>
> > >>> Thanks for the review.
> > >>>
> > >>> On Tue, Sep 26, 2017 at 10:47:40AM +0200, Hans Verkuil wrote:
> > >>>> On 26/09/17 00:25, Sakari Ailus wrote:
> > >>>>> v4l2_fwnode_reference_parse_int_prop() will find an fwnode such that under
> > >>>>> the device's own fwnode, it will follow child fwnodes with the given
> > >>>>> property-value pair and return the resulting fwnode.
> > >>>>>
> > >>>>> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > >>>>> ---
> > >>>>>  drivers/media/v4l2-core/v4l2-fwnode.c | 201 ++++++++++++++++++++++++++++++++++
> > >>>>>  1 file changed, 201 insertions(+)
> > >>>>>
> > >>>>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > >>>>> index f739dfd16cf7..f93049c361e4 100644
> > >>>>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > >>>>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > >>>>> @@ -578,6 +578,207 @@ static int v4l2_fwnode_reference_parse(
> > >>>>>  	return ret;
> > >>>>>  }
> > >>>>>  
> > >>>>> +/*
> > >>>>> + * v4l2_fwnode_reference_get_int_prop - parse a reference with integer
> > >>>>> + *					arguments
> > >>>>> + * @dev: struct device pointer
> > >>>>> + * @notifier: notifier for @dev
> > >>>>> + * @prop: the name of the property
> > >>>>> + * @index: the index of the reference to get
> > >>>>> + * @props: the array of integer property names
> > >>>>> + * @nprops: the number of integer property names in @nprops
> > >>>>> + *
> > >>>>> + * Find fwnodes referred to by a property @prop, then under that
> > >>>>> + * iteratively, @nprops times, follow each child node which has a
> > >>>>> + * property in @props array at a given child index the value of which
> > >>>>> + * matches the integer argument at an index.
> > >>>>
> > >>>> "at an index". Still makes no sense to me. Which index?
> > >>>
> > >>> How about this:
> > >>>
> > >>> First find an fwnode referred to by the reference at @index in @prop.
> > >>>
> > >>> Then under that fwnode, @nprops times, for each property in @props,
> > >>> iteratively follow child nodes starting from fwnode such that they have the
> > >>> property in @props array at the index of the child node distance from the
> > >>
> > >> distance? You mean 'instance'?
> > > 
> > > No. It's a tree structure: this is the distance between a node in the tree
> > > and the root node (i.e. device's fwnode).
> > > 
> > >>
> > >>> root node and the value of that property matching with the integer argument of
> > >>> the reference, at the same index.
> > >>
> > >> You've completely lost me. About halfway through this sentence my brain crashed :-)
> > > 
> > > :-D
> > > 
> > > Did keeping distance have any effect?
> > 
> > No :-)
> > 
> > "the index of the child node distance from the root node": I have absolutely
> > no idea how to interpret that.
> 
> This index is referring to the properties array and its value is the same
> as the distance of the child node from the device's root node.
> 
> > 
> > > 
> > >>
> > >>>
> > >>>>
> > >>>>> + *
> > >>>>> + * For example, if this function was called with arguments and values
> > >>>>> + * @dev corresponding to device "SEN", @prop == "flash-leds", @index
> > >>>>> + * == 1, @props == { "led" }, @nprops == 1, with the ASL snippet below
> > >>>>> + * it would return the node marked with THISONE. The @dev argument in
> > >>>>> + * the ASL below.
> > >>>>
> > >>>> I know I asked for this before, but can you change the example to one where
> > >>>> nprops = 2? I think that will help understanding this.
> > >>>
> > >>> I could do that but then the example no longer corresponds to any actual
> > >>> case that exists at the moment. LED nodes will use a single integer
> > >>> argument and lens-focus nodes none.
> > >>
> > >> So? The example is here to understand the code and it doesn't have to be
> > >> related to actual hardware for a mainlined driver.
> > > 
> > > This isn't about hardware, the definitions being parsed currently aren't
> > > specific to any single piece of hardware. I could add an example which does
> > > not exist, that's certainly possible. But I fail to see how it'd help
> > > while the contrary could well be the case.
> > 
> > It helps to relate the code (and the comments for that matter) to what is in
> > the ACPI. In fact, if you can make such an example, then I can see if I can
> > come up with a better description.
> 
> Hmm. I thought about the example, and figured out the graph data structure
> could be parsed using this function as well. From
> Documentation/acpi/dsd/graph.txt:
> 
>     Scope (\_SB.PCI0.I2C2)
>     {
> 	Device (CAM0)
> 	{
> 	    Name (_DSD, Package () {
> 		ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> 		Package () {
> 		    Package () { "compatible", Package () { "nokia,smia" } },
> 		},
> 		ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
> 		Package () {
> 		    Package () { "port0", "PRT0" },
> 		}
> 	    })
> 	    Name (PRT0, Package() {
> 		ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> 		Package () {
> 		    Package () { "port", 0 },
> 		},
> 		ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
> 		Package () {
> 		    Package () { "endpoint0", "EP00" },
> 		}
> 	    })
> 	    Name (EP00, Package() {
> 		ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> 		Package () {
> 		    Package () { "endpoint", 0 },
> 		    Package () { "remote-endpoint", Package() { \_SB.PCI0.ISP, 4, 0 } },
> 		}
> 	    })
> 	}
>     }
> 
>     Scope (\_SB.PCI0)
>     {
> 	Device (ISP)
> 	{
> 	    Name (_DSD, Package () {
> 		ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
> 		Package () {
> 		    Package () { "port4", "PRT4" },
> 		}
> 	    })
> 
> 	    Name (PRT4, Package() {
> 		ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> 		Package () {
> 		    Package () { "port", 4 }, /* CSI-2 port number */
> 		},
> 		ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
> 		Package () {
> 		    Package () { "endpoint0", "EP40" },
> 		}
> 	    })
> 
> 	    Name (EP40, Package() {
> 		ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> 		Package () {
> 		    Package () { "endpoint", 0 },
> 		    Package () { "remote-endpoint", Package () { \_SB.PCI0.I2C2.CAM0, 0, 0 } },
> 		}
> 	    })
> 	}
>     }

If you did this with DT it'd look roughly like:

cam0 {
	compatible = "nokia,smia";

	port {
		cam0_ep: endpoint {
			remote-endpoint = <&isp_port4_ep>;
		};
	};
};

isp {
	ports {
		port@4 {
			reg = <4>;
			isp_port4_ep: endpoint {
				remote-endpoint = <&cam0_ep>;
			};	
		};
	};
};

And in ACPI style, i.e. with explicit "port" and "endpoint" properties as
well as integers to point the ports and endpoints:

cam: cam0 {
	compatible = "nokia,smia";

	port {
		port = <0>;
		cam0_ep: endpoint {
			endpoint = <0>;
			remote-endpoint = <&isp 4 0>;
		};
	};
};

isp: isp {
	ports {
		port@4 {
			port = <4>;
			isp_port4_ep: endpoint {
				endpoint = <0>;
				remote-endpoint = <&cam 0 0>;
			};	
		};
	};
};


> 
> From the EP40 node under ISP device, you could parse the graph remote
> endpoint using v4l2_fwnode_reference_get_int_prop with these arguments (the
> argument dev changed to fwnode in an earlier version of the patch, I'll
> address that soon as well):
> 
>  @fwnode: fwnode referring to EP40 under ISP.
>  @prop: "remote-endpoint"
>  @index: 0
>  @props: "port", "endpoint"
>  @nprops: 2
> 
> And you'd get back fwnode referring to EP00 under CAM0.
> 
> The same works the other way around: if you use EP00 under CAM0 as the
> fwnode, you'll get fwnode referring to EP40 under ISP.
> 
> If the remote-endpoint property would have additional references beyond the
> first one, then incrementing index could be used to obtain them. The graph
> bindings don't allow this though.
> 
> This function can be eventually moved to the ACPI framework, but doing that
> right now would probably one kernel release delay for the functionality.
> 
> -- 
> Regards,
> 
> Sakari Ailus
> sakari.ailus@xxxxxxxxxxxxxxx

-- 
Sakari Ailus
e-mail: sakari.ailus@xxxxxx



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux