Re: [U-Boot] [PATCH 1/3] core: Add function to get device for ofnode

[Mario Six]

Hi Simon,

On Sun, Apr 22, 2018 at 10:13 PM, Simon Glass  wrote:
> Hi Mario,
>
> On 18 April 2018 at 02:20, Mario Six  wrote:
>> Hi Simon,
>>
>> On Thu, Apr 12, 2018 at 6:42 PM, Simon Glass  wrote:
>>> Hi Mario,
>>>
>>> On 10 April 2018 at 05:34, Mario Six  wrote:
 Hi Simon,

 On Fri, Mar 30, 2018 at 12:43 AM, Simon Glass  wrote:
> Hi Mario,
>
> On 28 March 2018 at 20:37, Mario Six  wrote:
>> It's sometimes useful to get the device associated with a given
> ofnode.
>> Implement a function to implement this lookup operation.
>
> Where would you use this? Can you not use phandles to find the device?
> Or uclass_get_device_by_ofnode() ?
>

 The function is used with the dev_{enable,disable}_by_path in the next
> patch:
 If I used any of the uclass_* functions or similar, the device would be
> probed,
 which is not what I want, since the device may not actually be
> physically
 present.
>>>
>>> So how about using uclass_find_device_by_ofnode() ?
>>>
>>
>> That would work for the disabling, true, but not for the enabling (which
> is
>> what is used in the upcoming board): Since the node is declared as
> disabled in
>> the DT, the device is not even bound (so uclass_find_device_by_ofnode)
> won't
>> return it.
>>
>> A more elegant solution would be to have device_probe check again if the
>> underlying ofnode is disabled, and stop the probing if that's the case.
> In this
>> scenario the disabled devices would still be displayed in the tree, but
> never
>> probed, which is probably OK (I don't know if there would be any side
> effects
>> with iterating through devices, for example). But changing the behavior
> of such
>> elementary API functions is probably not a good idea.
>
> That seems to be a different topic.
>
> Fundamentally I don't see the difference between
> uclass_find_device_by_ofnode() and your ofnode_dev().
>
> If you want to enable something after probing you will have to call
> device_bind() or similar. If that is your intent, I think you need a
> different function from ofnode_dev(), since it also relies on the device
> already being bound.
>

Ah, yes, I forgot that the *find* functions don't probe, sorry about that.

Yes, right, it won't be a problem to use uclass_find_device_by_ofnode
instead then.

> Regards,
> Simon
>

Best regards,
Mario
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 1/3] core: Add function to get device for ofnode

[Simon Glass]

Hi Mario,

On 18 April 2018 at 02:20, Mario Six  wrote:
> Hi Simon,
>
> On Thu, Apr 12, 2018 at 6:42 PM, Simon Glass  wrote:
>> Hi Mario,
>>
>> On 10 April 2018 at 05:34, Mario Six  wrote:
>>> Hi Simon,
>>>
>>> On Fri, Mar 30, 2018 at 12:43 AM, Simon Glass  wrote:
 Hi Mario,

 On 28 March 2018 at 20:37, Mario Six  wrote:
> It's sometimes useful to get the device associated with a given
ofnode.
> Implement a function to implement this lookup operation.

 Where would you use this? Can you not use phandles to find the device?
 Or uclass_get_device_by_ofnode() ?

>>>
>>> The function is used with the dev_{enable,disable}_by_path in the next
patch:
>>> If I used any of the uclass_* functions or similar, the device would be
probed,
>>> which is not what I want, since the device may not actually be
physically
>>> present.
>>
>> So how about using uclass_find_device_by_ofnode() ?
>>
>
> That would work for the disabling, true, but not for the enabling (which
is
> what is used in the upcoming board): Since the node is declared as
disabled in
> the DT, the device is not even bound (so uclass_find_device_by_ofnode)
won't
> return it.
>
> A more elegant solution would be to have device_probe check again if the
> underlying ofnode is disabled, and stop the probing if that's the case.
In this
> scenario the disabled devices would still be displayed in the tree, but
never
> probed, which is probably OK (I don't know if there would be any side
effects
> with iterating through devices, for example). But changing the behavior
of such
> elementary API functions is probably not a good idea.

That seems to be a different topic.

Fundamentally I don't see the difference between
uclass_find_device_by_ofnode() and your ofnode_dev().

If you want to enable something after probing you will have to call
device_bind() or similar. If that is your intent, I think you need a
different function from ofnode_dev(), since it also relies on the device
already being bound.

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 1/3] core: Add function to get device for ofnode

[Mario Six]

Hi Simon,

On Thu, Apr 12, 2018 at 6:42 PM, Simon Glass  wrote:
> Hi Mario,
>
> On 10 April 2018 at 05:34, Mario Six  wrote:
>> Hi Simon,
>>
>> On Fri, Mar 30, 2018 at 12:43 AM, Simon Glass  wrote:
>>> Hi Mario,
>>>
>>> On 28 March 2018 at 20:37, Mario Six  wrote:
 It's sometimes useful to get the device associated with a given ofnode.
 Implement a function to implement this lookup operation.
>>>
>>> Where would you use this? Can you not use phandles to find the device?
>>> Or uclass_get_device_by_ofnode() ?
>>>
>>
>> The function is used with the dev_{enable,disable}_by_path in the next patch:
>> If I used any of the uclass_* functions or similar, the device would be 
>> probed,
>> which is not what I want, since the device may not actually be physically
>> present.
>
> So how about using uclass_find_device_by_ofnode() ?
>

That would work for the disabling, true, but not for the enabling (which is
what is used in the upcoming board): Since the node is declared as disabled in
the DT, the device is not even bound (so uclass_find_device_by_ofnode) won't
return it.

A more elegant solution would be to have device_probe check again if the
underlying ofnode is disabled, and stop the probing if that's the case. In this
scenario the disabled devices would still be displayed in the tree, but never
probed, which is probably OK (I don't know if there would be any side effects
with iterating through devices, for example). But changing the behavior of such
elementary API functions is probably not a good idea.

> Regards,
> Simon

Best regards,
Mario
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 1/3] core: Add function to get device for ofnode

[Simon Glass]

Hi Mario,

On 10 April 2018 at 05:34, Mario Six  wrote:
> Hi Simon,
>
> On Fri, Mar 30, 2018 at 12:43 AM, Simon Glass  wrote:
>> Hi Mario,
>>
>> On 28 March 2018 at 20:37, Mario Six  wrote:
>>> It's sometimes useful to get the device associated with a given ofnode.
>>> Implement a function to implement this lookup operation.
>>
>> Where would you use this? Can you not use phandles to find the device?
>> Or uclass_get_device_by_ofnode() ?
>>
>
> The function is used with the dev_{enable,disable}_by_path in the next patch:
> If I used any of the uclass_* functions or similar, the device would be 
> probed,
> which is not what I want, since the device may not actually be physically
> present.

So how about using uclass_find_device_by_ofnode() ?

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 1/3] core: Add function to get device for ofnode

[Mario Six]

Hi Simon,

On Fri, Mar 30, 2018 at 12:43 AM, Simon Glass  wrote:
> Hi Mario,
>
> On 28 March 2018 at 20:37, Mario Six  wrote:
>> It's sometimes useful to get the device associated with a given ofnode.
>> Implement a function to implement this lookup operation.
>
> Where would you use this? Can you not use phandles to find the device?
> Or uclass_get_device_by_ofnode() ?
>

The function is used with the dev_{enable,disable}_by_path in the next patch:
If I used any of the uclass_* functions or similar, the device would be probed,
which is not what I want, since the device may not actually be physically
present.

>>
>> Signed-off-by: Mario Six 
>> ---
>>  drivers/core/ofnode.c | 15 +++
>>  include/dm/ofnode.h   |  8 
>>  2 files changed, 23 insertions(+)
>>
>> diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
>> index 4e4532651f..ca002063b3 100644
>> --- a/drivers/core/ofnode.c
>> +++ b/drivers/core/ofnode.c
>> @@ -16,6 +16,21 @@
>>  #include 
>>  #include 
>>
>> +struct udevice *ofnode_dev(ofnode node)
>
> Can you please add a test for this?
>
> This seems like an internal function since it does not probe the
> device. So how about putting it in device.h:
>
> device_get_by_ofnode() - does probe the device it returns
> device_find_by_ofnode() - doesn't probe
>
>> +{
>> +   struct uclass *uc;
>> +   struct udevice *dev;
>> +
>> +   list_for_each_entry(uc, >uclass_root, sibling_node) {
>> +   list_for_each_entry(dev, >dev_head, uclass_node) {
>> +   if (ofnode_equal(dev_ofnode(dev), node))
>> +   return dev;
>> +   }
>> +   }
>> +
>> +   return NULL;
>> +}
>> +
>>  int ofnode_read_u32(ofnode node, const char *propname, u32 *outp)
>>  {
>> assert(ofnode_valid(node));
>> diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h
>> index 0d008404f9..aec205eb80 100644
>> --- a/include/dm/ofnode.h
>> +++ b/include/dm/ofnode.h
>> @@ -193,6 +193,14 @@ static inline ofnode ofnode_null(void)
>> return node;
>>  }
>>
>> +/**
>> + * ofnode_dev() - Get the device associated with a given ofnode
>> + *
>> + * @node:  valid node reference to get the corresponding device for
>> + * @return a pointer to the udevice if OK, NULL on error
>> + */
>> +struct udevice *ofnode_dev(ofnode node);
>> +
>>  /**
>>   * ofnode_read_u32() - Read a 32-bit integer from a property
>>   *
>> --
>> 2.16.1
>>
>
> Regards,
> Simon
>

Everything else will be addressed in v2. Thanks for reviewing!

Best regards,

Mario
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 1/3] core: Add function to get device for ofnode

[Simon Glass]

Hi Mario,

On 28 March 2018 at 20:37, Mario Six  wrote:
> It's sometimes useful to get the device associated with a given ofnode.
> Implement a function to implement this lookup operation.

Where would you use this? Can you not use phandles to find the device?
Or uclass_get_device_by_ofnode() ?

>
> Signed-off-by: Mario Six 
> ---
>  drivers/core/ofnode.c | 15 +++
>  include/dm/ofnode.h   |  8 
>  2 files changed, 23 insertions(+)
>
> diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
> index 4e4532651f..ca002063b3 100644
> --- a/drivers/core/ofnode.c
> +++ b/drivers/core/ofnode.c
> @@ -16,6 +16,21 @@
>  #include 
>  #include 
>
> +struct udevice *ofnode_dev(ofnode node)

Can you please add a test for this?

This seems like an internal function since it does not probe the
device. So how about putting it in device.h:

device_get_by_ofnode() - does probe the device it returns
device_find_by_ofnode() - doesn't probe

> +{
> +   struct uclass *uc;
> +   struct udevice *dev;
> +
> +   list_for_each_entry(uc, >uclass_root, sibling_node) {
> +   list_for_each_entry(dev, >dev_head, uclass_node) {
> +   if (ofnode_equal(dev_ofnode(dev), node))
> +   return dev;
> +   }
> +   }
> +
> +   return NULL;
> +}
> +
>  int ofnode_read_u32(ofnode node, const char *propname, u32 *outp)
>  {
> assert(ofnode_valid(node));
> diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h
> index 0d008404f9..aec205eb80 100644
> --- a/include/dm/ofnode.h
> +++ b/include/dm/ofnode.h
> @@ -193,6 +193,14 @@ static inline ofnode ofnode_null(void)
> return node;
>  }
>
> +/**
> + * ofnode_dev() - Get the device associated with a given ofnode
> + *
> + * @node:  valid node reference to get the corresponding device for
> + * @return a pointer to the udevice if OK, NULL on error
> + */
> +struct udevice *ofnode_dev(ofnode node);
> +
>  /**
>   * ofnode_read_u32() - Read a 32-bit integer from a property
>   *
> --
> 2.16.1
>

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH 1/3] core: Add function to get device for ofnode

[Mario Six]

It's sometimes useful to get the device associated with a given ofnode.
Implement a function to implement this lookup operation.

Signed-off-by: Mario Six 
---
 drivers/core/ofnode.c | 15 +++
 include/dm/ofnode.h   |  8 
 2 files changed, 23 insertions(+)

diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
index 4e4532651f..ca002063b3 100644
--- a/drivers/core/ofnode.c
+++ b/drivers/core/ofnode.c
@@ -16,6 +16,21 @@
 #include 
 #include 
 
+struct udevice *ofnode_dev(ofnode node)
+{
+   struct uclass *uc;
+   struct udevice *dev;
+
+   list_for_each_entry(uc, >uclass_root, sibling_node) {
+   list_for_each_entry(dev, >dev_head, uclass_node) {
+   if (ofnode_equal(dev_ofnode(dev), node))
+   return dev;
+   }
+   }
+
+   return NULL;
+}
+
 int ofnode_read_u32(ofnode node, const char *propname, u32 *outp)
 {
assert(ofnode_valid(node));
diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h
index 0d008404f9..aec205eb80 100644
--- a/include/dm/ofnode.h
+++ b/include/dm/ofnode.h
@@ -193,6 +193,14 @@ static inline ofnode ofnode_null(void)
return node;
 }
 
+/**
+ * ofnode_dev() - Get the device associated with a given ofnode
+ *
+ * @node:  valid node reference to get the corresponding device for
+ * @return a pointer to the udevice if OK, NULL on error
+ */
+struct udevice *ofnode_dev(ofnode node);
+
 /**
  * ofnode_read_u32() - Read a 32-bit integer from a property
  *
-- 
2.16.1

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot