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
