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

[Sakari Ailus]

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 
> > > ---
> > >  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 

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

[Sakari Ailus]

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 
> > ---
> >  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 () { 

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

[Hans Verkuil]

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 
> ---
>  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.

> 
>>
>>>

> + *
> + * 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.

Regards,

Hans

> 
>>
>> If you really don't want to do this here, then put the example in the commit
>> log. I don't see any reason why you can't put it here, though.
>>
>> I think that once I see an 'nprops = 2' example I can rephrase that
>> brain-crash sentence for you...
>>
>> BTW, where are the ACPI 'bindings' defined anyway? For DT they are in the
>> bindings directory, but where does ACPI define such things? Just curious.
> 
> As the fwnode interface can be used to access information in both ACPI and
> DT, there is an incentive to maintain the interfaces effectively the same.
> In other words where the interfaces are the same, there is no need to
> define bindings for ACPI as such. Where there are differences the bindings
> are defined in Documentation/acpi/dsd .
> 
> The so far only technical reason to that is related to 

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

[Sakari Ailus]

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 
> >>> ---
> >>>  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?

> 
> > 
> >>
> >>> + *
> >>> + * 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.

> 
> If you really don't want to do this here, then put the example in the commit
> log. I don't see any reason why you can't put it here, though.
> 
> I think that once I see an 'nprops = 2' example I can rephrase that
> brain-crash sentence for you...
> 
> BTW, where are the ACPI 'bindings' defined anyway? For DT they are in the
> bindings directory, but where does ACPI define such things? Just curious.

As the fwnode interface can be used to access information in both ACPI and
DT, there is an incentive to maintain the interfaces effectively the same.
In other words where the interfaces are the same, there is no need to
define bindings for ACPI as such. Where there are differences the bindings
are defined in Documentation/acpi/dsd .

The so far only technical reason to that is related to the same is that
ACPI can only refer to device nodes (i.e. nodes that correspond to struct
devices), not sub-nodes under them.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi

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

[Hans Verkuil]

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 
>>> ---
>>>  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'?

> 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 
:-)

> 
>>
>>> + *
>>> + * 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.

If you really don't want to do this here, then put the example in the commit
log. I don't see any reason why you can't put it here, though.

I think that once I see an 'nprops = 2' example I can rephrase that
brain-crash sentence for you...

BTW, where are the ACPI 'bindings' defined anyway? For DT they are in the
bindings directory, but where does ACPI define such things? Just curious.

Regards,

Hans

> 
>>
>>> + *
>>> + * Device (LED)
>>> + * {
>>> + * Name (_DSD, Package () {
>>> + * ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
>>> + * Package () {
>>> + * Package () { "led0", "LED0" },
>>> + * Package () { "led1", "LED1" },
>>> + * }
>>> + * })
>>> + * Name (LED0, Package () {
>>> + * ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>>> + * Package () {
>>> + * Package () { "led", 0 },
>>> + * }
>>> + * })
>>> + * Name (LED1, Package () {
>>> + * // THISONE
>>> + * ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>>> + * Package () {
>>> + * Package () { "led", 1 },
>>> + * }
>>> + * })
>>> + * }
>>> + *
>>> + * Device (SEN)
>>> + * {
>>> + * Name (_DSD, Package () {
>>> + * ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>>> + * Package () {
>>> + * Package () {
>>> + * "flash-leds",
>>> + * Package () { ^LED, 0, ^LED, 1 },
>>> + * }
>>> + * }
>>> + * })
>>> + * }
>>> + *
>>> + * where
>>> + *
>>> + * LED LED driver device
>>> + * LED0First LED
>>> + * LED1Second LED
>>> + * 

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

[Sakari Ailus]

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 
> > ---
> >  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
root node and the value of that property matching with the integer argument of
the reference, at the same index.

> 
> > + *
> > + * 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.

> 
> > + *
> > + * Device (LED)
> > + * {
> > + * Name (_DSD, Package () {
> > + * ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
> > + * Package () {
> > + * Package () { "led0", "LED0" },
> > + * Package () { "led1", "LED1" },
> > + * }
> > + * })
> > + * Name (LED0, Package () {
> > + * ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > + * Package () {
> > + * Package () { "led", 0 },
> > + * }
> > + * })
> > + * Name (LED1, Package () {
> > + * // THISONE
> > + * ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > + * Package () {
> > + * Package () { "led", 1 },
> > + * }
> > + * })
> > + * }
> > + *
> > + * Device (SEN)
> > + * {
> > + * Name (_DSD, Package () {
> > + * ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > + * Package () {
> > + * Package () {
> > + * "flash-leds",
> > + * Package () { ^LED, 0, ^LED, 1 },
> > + * }
> > + * }
> > + * })
> > + * }
> > + *
> > + * where
> > + *
> > + * LED LED driver device
> > + * LED0First LED
> > + * LED1Second LED
> > + * SEN Camera sensor device (or another device the LED is
> > + * related to)
> > + *
> > + * Return: 0 on success
> > + *-ENOENT if no entries (or the property itself) were found
> > + *-EINVAL if property parsing otherwise failed
> > + *-ENOMEM if memory allocation failed
> > + */
> > +static struct fwnode_handle *v4l2_fwnode_reference_get_int_prop(
> > +   struct fwnode_handle *fwnode, const char *prop, unsigned int index,
> > +   const char **props, unsigned int nprops)
> > +{
> > +   struct fwnode_reference_args fwnode_args;
> > +   unsigned int *args = fwnode_args.args;
> > +   struct fwnode_handle *child;
> > +   int ret;
> > +
> > +   /*
> > +* Obtain remote fwnode as well as the integer arguments.
> > +*
> > +* Note that right now both -ENODATA 

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

[Hans Verkuil]

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 
> ---
>  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?

> + *
> + * 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.

> + *
> + *   Device (LED)
> + *   {
> + *   Name (_DSD, Package () {
> + *   ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
> + *   Package () {
> + *   Package () { "led0", "LED0" },
> + *   Package () { "led1", "LED1" },
> + *   }
> + *   })
> + *   Name (LED0, Package () {
> + *   ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> + *   Package () {
> + *   Package () { "led", 0 },
> + *   }
> + *   })
> + *   Name (LED1, Package () {
> + *   // THISONE
> + *   ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> + *   Package () {
> + *   Package () { "led", 1 },
> + *   }
> + *   })
> + *   }
> + *
> + *   Device (SEN)
> + *   {
> + *   Name (_DSD, Package () {
> + *   ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> + *   Package () {
> + *   Package () {
> + *   "flash-leds",
> + *   Package () { ^LED, 0, ^LED, 1 },
> + *   }
> + *   }
> + *   })
> + *   }
> + *
> + * where
> + *
> + *   LED LED driver device
> + *   LED0First LED
> + *   LED1Second LED
> + *   SEN Camera sensor device (or another device the LED is
> + *   related to)
> + *
> + * Return: 0 on success
> + *  -ENOENT if no entries (or the property itself) were found
> + *  -EINVAL if property parsing otherwise failed
> + *  -ENOMEM if memory allocation failed
> + */
> +static struct fwnode_handle *v4l2_fwnode_reference_get_int_prop(
> + struct fwnode_handle *fwnode, const char *prop, unsigned int index,
> + const char **props, unsigned int nprops)
> +{
> + struct fwnode_reference_args fwnode_args;
> + unsigned int *args = fwnode_args.args;
> + struct fwnode_handle *child;
> + int ret;
> +
> + /*
> +  * Obtain remote fwnode as well as the integer arguments.
> +  *
> +  * Note that right now both -ENODATA and -ENOENT may signal
> +  * out-of-bounds access. Return -ENOENT in that case.
> +  */
> + ret = fwnode_property_get_reference_args(fwnode, prop, NULL, nprops,
> +  index, _args);
> + if (ret)
> + return ERR_PTR(ret == -ENODATA ? -ENOENT : ret);
> +
> + /*
> +  * Find a node in the tree under the referred fwnode corresponding the

corresponding -> corresponding to

> +  * integer arguments.
> +  */
> + fwnode = fwnode_args.fwnode;
> + while (nprops--) {
> + u32 val;
> +
> + /* Loop over all child nodes under fwnode. */
> + fwnode_for_each_child_node(fwnode, child) {
> + if (fwnode_property_read_u32(child, *props, ))
> +   

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

[Sakari Ailus]

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 
---
 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.
+ *
+ * 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.
+ *
+ * Device (LED)
+ * {
+ * Name (_DSD, Package () {
+ * ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
+ * Package () {
+ * Package () { "led0", "LED0" },
+ * Package () { "led1", "LED1" },
+ * }
+ * })
+ * Name (LED0, Package () {
+ * ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ * Package () {
+ * Package () { "led", 0 },
+ * }
+ * })
+ * Name (LED1, Package () {
+ * // THISONE
+ * ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ * Package () {
+ * Package () { "led", 1 },
+ * }
+ * })
+ * }
+ *
+ * Device (SEN)
+ * {
+ * Name (_DSD, Package () {
+ * ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ * Package () {
+ * Package () {
+ * "flash-leds",
+ * Package () { ^LED, 0, ^LED, 1 },
+ * }
+ * }
+ * })
+ * }
+ *
+ * where
+ *
+ * LED LED driver device
+ * LED0First LED
+ * LED1Second LED
+ * SEN Camera sensor device (or another device the LED is
+ * related to)
+ *
+ * Return: 0 on success
+ *-ENOENT if no entries (or the property itself) were found
+ *-EINVAL if property parsing otherwise failed
+ *-ENOMEM if memory allocation failed
+ */
+static struct fwnode_handle *v4l2_fwnode_reference_get_int_prop(
+   struct fwnode_handle *fwnode, const char *prop, unsigned int index,
+   const char **props, unsigned int nprops)
+{
+   struct fwnode_reference_args fwnode_args;
+   unsigned int *args = fwnode_args.args;
+   struct fwnode_handle *child;
+   int ret;
+
+   /*
+* Obtain remote fwnode as well as the integer arguments.
+*
+* Note that right now both -ENODATA and -ENOENT may signal
+* out-of-bounds access. Return -ENOENT in that case.
+*/
+   ret = fwnode_property_get_reference_args(fwnode, prop, NULL, nprops,
+index, _args);
+   if (ret)
+   return ERR_PTR(ret == -ENODATA ? -ENOENT : ret);
+
+   /*
+* Find a node in the tree under the referred fwnode corresponding the
+* integer arguments.
+*/
+   fwnode = fwnode_args.fwnode;
+   while (nprops--) {
+   u32 val;
+
+   /* Loop over all child nodes under fwnode. */
+   fwnode_for_each_child_node(fwnode, child) {
+   if (fwnode_property_read_u32(child, *props, ))
+   continue;
+
+   /* Found property, see if its value matches. */
+   if (val == *args)
+   break;
+   }
+
+   fwnode_handle_put(fwnode);
+
+   /* No property found; return an error here. */
+   if (!child) {
+   fwnode =