mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH 1/2] of: implement new of_device_ensured_probed_by_alias_stem
@ 2022-03-15 13:39 Andrej Picej
  2022-03-15 13:39 ` [PATCH 2/2] mfd: da9063: ensure all gpio devices are probed before Andrej Picej
  2022-03-15 14:16 ` [PATCH 1/2] of: implement new of_device_ensured_probed_by_alias_stem Ahmad Fatoum
  0 siblings, 2 replies; 9+ messages in thread
From: Andrej Picej @ 2022-03-15 13:39 UTC (permalink / raw)
  To: barebox

Function first goes through all the aliases which have the given stem.
It then ensures that all the devices hiding under these aliases are
probed.

Signed-off-by: Andrej Picej <andrej.picej@norik.com>
---
 drivers/of/base.c | 28 ++++++++++++++++++++++++++++
 include/of.h      |  6 ++++++
 2 files changed, 34 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 80465d6d50..055dba97ab 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -274,6 +274,34 @@ int of_alias_get_id_from(struct device_node *root, struct device_node *np,
 }
 EXPORT_SYMBOL_GPL(of_alias_get_id_from);
 
+/**
+ * of_device_ensured_probed_by_alias_stem - Ensure all devices with alias base name
+ * are probed
+ * @stem:	Alias stem of the given device_node
+ *
+ * The function ensures all devices with the given alias stem are probed.
+ *
+ * Returns 0 on success or error code.
+ */
+int of_device_ensured_probed_by_alias_stem(const char *stem)
+{
+	struct alias_prop *app;
+	int id = -ENODEV;
+	int ret;
+
+	list_for_each_entry(app, &aliases_lookup, link) {
+		if (of_node_cmp(app->stem, stem) != 0)
+			continue;
+
+		ret = of_device_ensure_probed(app->np);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_device_ensured_probed_by_alias_stem);
+
 const char *of_alias_get(struct device_node *np)
 {
 	struct alias_prop *app;
diff --git a/include/of.h b/include/of.h
index 216d0ee763..95817c58a4 100644
--- a/include/of.h
+++ b/include/of.h
@@ -261,6 +261,7 @@ extern void of_alias_scan(void);
 extern int of_alias_get_id(struct device_node *np, const char *stem);
 extern int of_alias_get_id_from(struct device_node *root, struct device_node *np,
 				const char *stem);
+extern int of_device_ensured_probed_by_alias_stem(const char *stem);
 extern const char *of_alias_get(struct device_node *np);
 extern int of_modalias_node(struct device_node *node, char *modalias, int len);
 
@@ -750,6 +751,11 @@ static inline int of_alias_get_id_from(struct device_node *root, struct device_n
 	return -ENOSYS;
 }
 
+static int of_device_ensured_probed_by_aliases(const char *stem)
+{
+	return -ENOSYS;
+}
+
 static inline const char *of_alias_get(struct device_node *np)
 {
 	return NULL;
-- 
2.25.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


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

* [PATCH 2/2] mfd: da9063: ensure all gpio devices are probed before
  2022-03-15 13:39 [PATCH 1/2] of: implement new of_device_ensured_probed_by_alias_stem Andrej Picej
@ 2022-03-15 13:39 ` Andrej Picej
  2022-03-15 14:24   ` Ahmad Fatoum
  2022-03-15 14:16 ` [PATCH 1/2] of: implement new of_device_ensured_probed_by_alias_stem Ahmad Fatoum
  1 sibling, 1 reply; 9+ messages in thread
From: Andrej Picej @ 2022-03-15 13:39 UTC (permalink / raw)
  To: barebox

GPIO lines in da9063 are assigned dynamically, while majority of SOC
GPIO drivers assign their GPIOs in static manner (GPIO line numbers can
be calculated).

This introduces regression if deep probe support is used. If da9063
GPIOs are registered before the SOCs GPIOs, there is a good chance that
the SOCs statically computed GPIO line numbers will already be used by
PMIC.

Ensure all SOCs GPIO drivers and GPIO lines get registered before the
da9063 registers its own gpiochip.

Signed-off-by: Andrej Picej <andrej.picej@norik.com>
---
 drivers/mfd/da9063.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mfd/da9063.c b/drivers/mfd/da9063.c
index a4e5226f3c..8b943eb4ef 100644
--- a/drivers/mfd/da9063.c
+++ b/drivers/mfd/da9063.c
@@ -390,6 +390,10 @@ static int da9063_probe(struct device_d *dev)
 	restart_handler_register(&priv->restart);
 
 	if (IS_ENABLED(CONFIG_GPIOLIB)) {
+		ret = of_device_ensured_probed_by_alias_stem("gpio");
+		if (ret)
+			goto on_error;
+
 		priv->gpio.base = -1;
 		priv->gpio.ngpio = 5;
 		priv->gpio.ops  = &da9063_gpio_ops;
-- 
2.25.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


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

* Re: [PATCH 1/2] of: implement new of_device_ensured_probed_by_alias_stem
  2022-03-15 13:39 [PATCH 1/2] of: implement new of_device_ensured_probed_by_alias_stem Andrej Picej
  2022-03-15 13:39 ` [PATCH 2/2] mfd: da9063: ensure all gpio devices are probed before Andrej Picej
@ 2022-03-15 14:16 ` Ahmad Fatoum
  2022-03-15 14:29   ` Ahmad Fatoum
  2022-03-16  7:29   ` Andrej Picej
  1 sibling, 2 replies; 9+ messages in thread
From: Ahmad Fatoum @ 2022-03-15 14:16 UTC (permalink / raw)
  To: Andrej Picej, barebox

Hello Andrej,

On 15.03.22 14:39, Andrej Picej wrote:
> Function first goes through all the aliases which have the given stem.
> It then ensures that all the devices hiding under these aliases are
> probed.
> 
> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> ---
>  drivers/of/base.c | 28 ++++++++++++++++++++++++++++
>  include/of.h      |  6 ++++++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 80465d6d50..055dba97ab 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -274,6 +274,34 @@ int of_alias_get_id_from(struct device_node *root, struct device_node *np,
>  }
>  EXPORT_SYMBOL_GPL(of_alias_get_id_from);
>  
> +/**
> + * of_device_ensured_probed_by_alias_stem - Ensure all devices with alias base name
> + * are probed
> + * @stem:	Alias stem of the given device_node
> + *
> + * The function ensures all devices with the given alias stem are probed.
> + *
> + * Returns 0 on success or error code.
> + */
> +int of_device_ensured_probed_by_alias_stem(const char *stem)
> +{
> +	struct alias_prop *app;
> +	int id = -ENODEV;
> +	int ret;

Can you early exit here if (!deep_probe_is_supported())?

This saves non-deep probe users the hassle of needless iteration
and string comparisons.

> +
> +	list_for_each_entry(app, &aliases_lookup, link) {
> +		if (of_node_cmp(app->stem, stem) != 0)
> +			continue;
> +
> +		ret = of_device_ensure_probed(app->np);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_device_ensured_probed_by_alias_stem);
> +
>  const char *of_alias_get(struct device_node *np)
>  {
>  	struct alias_prop *app;
> diff --git a/include/of.h b/include/of.h
> index 216d0ee763..95817c58a4 100644
> --- a/include/of.h
> +++ b/include/of.h
> @@ -261,6 +261,7 @@ extern void of_alias_scan(void);
>  extern int of_alias_get_id(struct device_node *np, const char *stem);
>  extern int of_alias_get_id_from(struct device_node *root, struct device_node *np,
>  				const char *stem);
> +extern int of_device_ensured_probed_by_alias_stem(const char *stem);
>  extern const char *of_alias_get(struct device_node *np);
>  extern int of_modalias_node(struct device_node *node, char *modalias, int len);
>  
> @@ -750,6 +751,11 @@ static inline int of_alias_get_id_from(struct device_node *root, struct device_n
>  	return -ENOSYS;
>  }
>  
> +static int of_device_ensured_probed_by_aliases(const char *stem)
> +{
> +	return -ENOSYS;
> +}
> +
>  static inline const char *of_alias_get(struct device_node *np)
>  {
>  	return NULL;


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


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

* Re: [PATCH 2/2] mfd: da9063: ensure all gpio devices are probed before
  2022-03-15 13:39 ` [PATCH 2/2] mfd: da9063: ensure all gpio devices are probed before Andrej Picej
@ 2022-03-15 14:24   ` Ahmad Fatoum
  2022-03-16 10:44     ` Sascha Hauer
  0 siblings, 1 reply; 9+ messages in thread
From: Ahmad Fatoum @ 2022-03-15 14:24 UTC (permalink / raw)
  To: Andrej Picej, barebox

On 15.03.22 14:39, Andrej Picej wrote:
> GPIO lines in da9063 are assigned dynamically, while majority of SOC
> GPIO drivers assign their GPIOs in static manner (GPIO line numbers can
> be calculated).
> 
> This introduces regression if deep probe support is used. If da9063
> GPIOs are registered before the SOCs GPIOs, there is a good chance that
> the SOCs statically computed GPIO line numbers will already be used by
> PMIC.
> 
> Ensure all SOCs GPIO drivers and GPIO lines get registered before the
> da9063 registers its own gpiochip.
> 
> Signed-off-by: Andrej Picej <andrej.picej@norik.com>

I don't think this is the proper place. Also I think you may have
introduced a recursion here if the PMIC happens to have an alias
itself?

What I think we could do instead is to move this into gpiochip_add()
and opencode the alias iterator:


foreach (gpio_alias) {
	if (gpio_alias_dev == chip->dev)
		__gpiochip_add();
	else
		of_device_ensure_probed();
}

I don't know if that's the cleanest way, but that's what came to my mind.

Cheers,
Ahmad

> ---
>  drivers/mfd/da9063.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/mfd/da9063.c b/drivers/mfd/da9063.c
> index a4e5226f3c..8b943eb4ef 100644
> --- a/drivers/mfd/da9063.c
> +++ b/drivers/mfd/da9063.c
> @@ -390,6 +390,10 @@ static int da9063_probe(struct device_d *dev)
>  	restart_handler_register(&priv->restart);
>  
>  	if (IS_ENABLED(CONFIG_GPIOLIB)) {
> +		ret = of_device_ensured_probed_by_alias_stem("gpio");
> +		if (ret)
> +			goto on_error;
> +
>  		priv->gpio.base = -1;
>  		priv->gpio.ngpio = 5;
>  		priv->gpio.ops  = &da9063_gpio_ops;


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


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

* Re: [PATCH 1/2] of: implement new of_device_ensured_probed_by_alias_stem
  2022-03-15 14:16 ` [PATCH 1/2] of: implement new of_device_ensured_probed_by_alias_stem Ahmad Fatoum
@ 2022-03-15 14:29   ` Ahmad Fatoum
  2022-03-16 10:01     ` Andrej Picej
  2022-03-16  7:29   ` Andrej Picej
  1 sibling, 1 reply; 9+ messages in thread
From: Ahmad Fatoum @ 2022-03-15 14:29 UTC (permalink / raw)
  To: Andrej Picej, barebox

On 15.03.22 15:16, Ahmad Fatoum wrote:
> Hello Andrej,
> 
> On 15.03.22 14:39, Andrej Picej wrote:
>> Function first goes through all the aliases which have the given stem.
>> It then ensures that all the devices hiding under these aliases are
>> probed.
>>
>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>> ---
>>  drivers/of/base.c | 28 ++++++++++++++++++++++++++++
>>  include/of.h      |  6 ++++++
>>  2 files changed, 34 insertions(+)
>>
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index 80465d6d50..055dba97ab 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -274,6 +274,34 @@ int of_alias_get_id_from(struct device_node *root, struct device_node *np,
>>  }
>>  EXPORT_SYMBOL_GPL(of_alias_get_id_from);
>>  
>> +/**
>> + * of_device_ensured_probed_by_alias_stem - Ensure all devices with alias base name
>> + * are probed
>> + * @stem:	Alias stem of the given device_node
>> + *
>> + * The function ensures all devices with the given alias stem are probed.
>> + *
>> + * Returns 0 on success or error code.
>> + */
>> +int of_device_ensured_probed_by_alias_stem(const char *stem)
>> +{
>> +	struct alias_prop *app;
>> +	int id = -ENODEV;
>> +	int ret;
> 
> Can you early exit here if (!deep_probe_is_supported())?
> 
> This saves non-deep probe users the hassle of needless iteration
> and string comparisons.
> 
>> +
>> +	list_for_each_entry(app, &aliases_lookup, link) {

Corner case: aliases in DT are not sorted, so 

&{/aliases} { gpio1 = &pmic_gpio; gpio0 = &gpio1; };

may result in pmic_gpio being probed first..

Could be resolved by sorting aliases on insertion though. hmm..

>> +		if (of_node_cmp(app->stem, stem) != 0)
>> +			continue;
>> +
>> +		ret = of_device_ensure_probed(app->np);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(of_device_ensured_probed_by_alias_stem);
>> +
>>  const char *of_alias_get(struct device_node *np)
>>  {
>>  	struct alias_prop *app;
>> diff --git a/include/of.h b/include/of.h
>> index 216d0ee763..95817c58a4 100644
>> --- a/include/of.h
>> +++ b/include/of.h
>> @@ -261,6 +261,7 @@ extern void of_alias_scan(void);
>>  extern int of_alias_get_id(struct device_node *np, const char *stem);
>>  extern int of_alias_get_id_from(struct device_node *root, struct device_node *np,
>>  				const char *stem);
>> +extern int of_device_ensured_probed_by_alias_stem(const char *stem);
>>  extern const char *of_alias_get(struct device_node *np);
>>  extern int of_modalias_node(struct device_node *node, char *modalias, int len);
>>  
>> @@ -750,6 +751,11 @@ static inline int of_alias_get_id_from(struct device_node *root, struct device_n
>>  	return -ENOSYS;
>>  }
>>  
>> +static int of_device_ensured_probed_by_aliases(const char *stem)
>> +{
>> +	return -ENOSYS;
>> +}
>> +
>>  static inline const char *of_alias_get(struct device_node *np)
>>  {
>>  	return NULL;
> 
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


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

* Re: [PATCH 1/2] of: implement new of_device_ensured_probed_by_alias_stem
  2022-03-15 14:16 ` [PATCH 1/2] of: implement new of_device_ensured_probed_by_alias_stem Ahmad Fatoum
  2022-03-15 14:29   ` Ahmad Fatoum
@ 2022-03-16  7:29   ` Andrej Picej
  1 sibling, 0 replies; 9+ messages in thread
From: Andrej Picej @ 2022-03-16  7:29 UTC (permalink / raw)
  To: Ahmad Fatoum, barebox


On 15. 03. 22 15:16, Ahmad Fatoum wrote:
> Hello Andrej,
> 
> On 15.03.22 14:39, Andrej Picej wrote:
>> Function first goes through all the aliases which have the given stem.
>> It then ensures that all the devices hiding under these aliases are
>> probed.
>>
>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>> ---
>>   drivers/of/base.c | 28 ++++++++++++++++++++++++++++
>>   include/of.h      |  6 ++++++
>>   2 files changed, 34 insertions(+)
>>
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index 80465d6d50..055dba97ab 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -274,6 +274,34 @@ int of_alias_get_id_from(struct device_node *root, struct device_node *np,
>>   }
>>   EXPORT_SYMBOL_GPL(of_alias_get_id_from);
>>   
>> +/**
>> + * of_device_ensured_probed_by_alias_stem - Ensure all devices with alias base name
>> + * are probed
>> + * @stem:	Alias stem of the given device_node
>> + *
>> + * The function ensures all devices with the given alias stem are probed.
>> + *
>> + * Returns 0 on success or error code.
>> + */
>> +int of_device_ensured_probed_by_alias_stem(const char *stem)
>> +{
>> +	struct alias_prop *app;
>> +	int id = -ENODEV;
>> +	int ret;
> 
> Can you early exit here if (!deep_probe_is_supported())?
> 
> This saves non-deep probe users the hassle of needless iteration
> and string comparisons.
> 

Yes of course, I forgot about that.

>> +
>> +	list_for_each_entry(app, &aliases_lookup, link) {
>> +		if (of_node_cmp(app->stem, stem) != 0)
>> +			continue;
>> +
>> +		ret = of_device_ensure_probed(app->np);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(of_device_ensured_probed_by_alias_stem);
>> +
>>   const char *of_alias_get(struct device_node *np)
>>   {
>>   	struct alias_prop *app;
>> diff --git a/include/of.h b/include/of.h
>> index 216d0ee763..95817c58a4 100644
>> --- a/include/of.h
>> +++ b/include/of.h
>> @@ -261,6 +261,7 @@ extern void of_alias_scan(void);
>>   extern int of_alias_get_id(struct device_node *np, const char *stem);
>>   extern int of_alias_get_id_from(struct device_node *root, struct device_node *np,
>>   				const char *stem);
>> +extern int of_device_ensured_probed_by_alias_stem(const char *stem);
>>   extern const char *of_alias_get(struct device_node *np);
>>   extern int of_modalias_node(struct device_node *node, char *modalias, int len);
>>   
>> @@ -750,6 +751,11 @@ static inline int of_alias_get_id_from(struct device_node *root, struct device_n
>>   	return -ENOSYS;
>>   }
>>   
>> +static int of_device_ensured_probed_by_aliases(const char *stem)
>> +{
>> +	return -ENOSYS;
>> +}
>> +
>>   static inline const char *of_alias_get(struct device_node *np)
>>   {
>>   	return NULL;
> 
> 

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


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

* Re: [PATCH 1/2] of: implement new of_device_ensured_probed_by_alias_stem
  2022-03-15 14:29   ` Ahmad Fatoum
@ 2022-03-16 10:01     ` Andrej Picej
  0 siblings, 0 replies; 9+ messages in thread
From: Andrej Picej @ 2022-03-16 10:01 UTC (permalink / raw)
  To: Ahmad Fatoum, barebox



On 15. 03. 22 15:29, Ahmad Fatoum wrote:
> On 15.03.22 15:16, Ahmad Fatoum wrote:
>> Hello Andrej,
>>
>> On 15.03.22 14:39, Andrej Picej wrote:
>>> Function first goes through all the aliases which have the given stem.
>>> It then ensures that all the devices hiding under these aliases are
>>> probed.
>>>
>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>>> ---
>>>   drivers/of/base.c | 28 ++++++++++++++++++++++++++++
>>>   include/of.h      |  6 ++++++
>>>   2 files changed, 34 insertions(+)
>>>
>>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>>> index 80465d6d50..055dba97ab 100644
>>> --- a/drivers/of/base.c
>>> +++ b/drivers/of/base.c
>>> @@ -274,6 +274,34 @@ int of_alias_get_id_from(struct device_node *root, struct device_node *np,
>>>   }
>>>   EXPORT_SYMBOL_GPL(of_alias_get_id_from);
>>>   
>>> +/**
>>> + * of_device_ensured_probed_by_alias_stem - Ensure all devices with alias base name
>>> + * are probed
>>> + * @stem:	Alias stem of the given device_node
>>> + *
>>> + * The function ensures all devices with the given alias stem are probed.
>>> + *
>>> + * Returns 0 on success or error code.
>>> + */
>>> +int of_device_ensured_probed_by_alias_stem(const char *stem)
>>> +{
>>> +	struct alias_prop *app;
>>> +	int id = -ENODEV;
>>> +	int ret;
>>
>> Can you early exit here if (!deep_probe_is_supported())?
>>
>> This saves non-deep probe users the hassle of needless iteration
>> and string comparisons.
>>
>>> +
>>> +	list_for_each_entry(app, &aliases_lookup, link) {
> 
> Corner case: aliases in DT are not sorted, so
> 
> &{/aliases} { gpio1 = &pmic_gpio; gpio0 = &gpio1; };
> 
> may result in pmic_gpio being probed first..
True
> 
> Could be resolved by sorting aliases on insertion though. hmm..

Ok I will check things up and send a v2.

Thanks for review.
> 
>>> +		if (of_node_cmp(app->stem, stem) != 0)
>>> +			continue;
>>> +
>>> +		ret = of_device_ensure_probed(app->np);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(of_device_ensured_probed_by_alias_stem);
>>> +
>>>   const char *of_alias_get(struct device_node *np)
>>>   {
>>>   	struct alias_prop *app;
>>> diff --git a/include/of.h b/include/of.h
>>> index 216d0ee763..95817c58a4 100644
>>> --- a/include/of.h
>>> +++ b/include/of.h
>>> @@ -261,6 +261,7 @@ extern void of_alias_scan(void);
>>>   extern int of_alias_get_id(struct device_node *np, const char *stem);
>>>   extern int of_alias_get_id_from(struct device_node *root, struct device_node *np,
>>>   				const char *stem);
>>> +extern int of_device_ensured_probed_by_alias_stem(const char *stem);
>>>   extern const char *of_alias_get(struct device_node *np);
>>>   extern int of_modalias_node(struct device_node *node, char *modalias, int len);
>>>   
>>> @@ -750,6 +751,11 @@ static inline int of_alias_get_id_from(struct device_node *root, struct device_n
>>>   	return -ENOSYS;
>>>   }
>>>   
>>> +static int of_device_ensured_probed_by_aliases(const char *stem)
>>> +{
>>> +	return -ENOSYS;
>>> +}
>>> +
>>>   static inline const char *of_alias_get(struct device_node *np)
>>>   {
>>>   	return NULL;
>>
>>
> 
> 

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


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

* Re: [PATCH 2/2] mfd: da9063: ensure all gpio devices are probed before
  2022-03-15 14:24   ` Ahmad Fatoum
@ 2022-03-16 10:44     ` Sascha Hauer
  2022-03-21  9:46       ` Andrej Picej
  0 siblings, 1 reply; 9+ messages in thread
From: Sascha Hauer @ 2022-03-16 10:44 UTC (permalink / raw)
  To: Ahmad Fatoum; +Cc: Andrej Picej, barebox

On Tue, Mar 15, 2022 at 03:24:23PM +0100, Ahmad Fatoum wrote:
> On 15.03.22 14:39, Andrej Picej wrote:
> > GPIO lines in da9063 are assigned dynamically, while majority of SOC
> > GPIO drivers assign their GPIOs in static manner (GPIO line numbers can
> > be calculated).
> > 
> > This introduces regression if deep probe support is used. If da9063
> > GPIOs are registered before the SOCs GPIOs, there is a good chance that
> > the SOCs statically computed GPIO line numbers will already be used by
> > PMIC.
> > 
> > Ensure all SOCs GPIO drivers and GPIO lines get registered before the
> > da9063 registers its own gpiochip.
> > 
> > Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> 
> I don't think this is the proper place. Also I think you may have
> introduced a recursion here if the PMIC happens to have an alias
> itself?
> 
> What I think we could do instead is to move this into gpiochip_add()
> and opencode the alias iterator:
> 
> 
> foreach (gpio_alias) {
> 	if (gpio_alias_dev == chip->dev)
> 		__gpiochip_add();
> 	else
> 		of_device_ensure_probed();
> }

This of_device_ensure_probed() will bring you into the very same loop
again. Better call this loop from outside gpiochip_add(). We could
add a gpio_ensure_probed() which executes this loop. That would be
called from some initcall, or maybe from board code when gpios are
needed before this initcall.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


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

* Re: [PATCH 2/2] mfd: da9063: ensure all gpio devices are probed before
  2022-03-16 10:44     ` Sascha Hauer
@ 2022-03-21  9:46       ` Andrej Picej
  0 siblings, 0 replies; 9+ messages in thread
From: Andrej Picej @ 2022-03-21  9:46 UTC (permalink / raw)
  To: Sascha Hauer, Ahmad Fatoum; +Cc: barebox



On 16. 03. 22 11:44, Sascha Hauer wrote:
> On Tue, Mar 15, 2022 at 03:24:23PM +0100, Ahmad Fatoum wrote:
>> On 15.03.22 14:39, Andrej Picej wrote:
>>> GPIO lines in da9063 are assigned dynamically, while majority of SOC
>>> GPIO drivers assign their GPIOs in static manner (GPIO line numbers can
>>> be calculated).
>>>
>>> This introduces regression if deep probe support is used. If da9063
>>> GPIOs are registered before the SOCs GPIOs, there is a good chance that
>>> the SOCs statically computed GPIO line numbers will already be used by
>>> PMIC.
>>>
>>> Ensure all SOCs GPIO drivers and GPIO lines get registered before the
>>> da9063 registers its own gpiochip.
>>>
>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>>
>> I don't think this is the proper place. Also I think you may have
>> introduced a recursion here if the PMIC happens to have an alias
>> itself?
>>
>> What I think we could do instead is to move this into gpiochip_add()
>> and opencode the alias iterator:
>>
>>
>> foreach (gpio_alias) {
>> 	if (gpio_alias_dev == chip->dev)
>> 		__gpiochip_add();
>> 	else
>> 		of_device_ensure_probed();
>> }
> 
> This of_device_ensure_probed() will bring you into the very same loop
> again. Better call this loop from outside gpiochip_add(). We could
> add a gpio_ensure_probed() which executes this loop. That would be
> called from some initcall, or maybe from board code when gpios are
> needed before this initcall.
> 
> Sascha
> 

That was my initial idea, but then I wanted to make it a bit more 
general, so it could be used also for other, "non gpio" devices.
Would that make sense?

I would expend the function (from PATCH 1/1) into something like this:
> int of_device_ensured_probed_by_alias_stem(struct device_node *np, const char *stem)
> {
> 	struct alias_prop *app;
> 	int id = -ENODEV;
> 	int ret;
> 
> 	if (!deep_probe_is_supported())
> 		return 0;
> 
> 	list_for_each_entry(app, &aliases_lookup, link) {
> 		if (of_node_cmp(app->stem, stem) != 0)
> 			continue;
> 
> 		/* If device node is given skip the probing of that device so we
> 		 * avoid recursion.
> 		 */
> 		if (np != NULL && np == app->np)
> 			continue;
> 
> 		ret = of_device_ensure_probed(app->np);
> 		if (ret)
> 			return ret;
> 	}
> 
> 	return 0;
> }
> EXPORT_SYMBOL_GPL(of_device_ensured_probed_by_alias_stem);

Where the user can specify which device should be skipped, so the method 
doesn't introduce recursion if device happens to have an alias.

We could then call this function from board file like this (example for 
PHYTEC):
> --- a/arch/arm/boards/phytec-som-imx6/board.c
> +++ b/arch/arm/boards/phytec-som-imx6/board.c
> @@ -111,6 +111,13 @@ static int phycore_da9062_setup_buck_mode(void)
>         if (!pmic_np)
>                 return -ENODEV;
>  
> +       of_device_ensured_probed_by_alias_stem(pmic_np, "gpio");
> +
>         ret = of_device_ensure_probed(pmic_np);
>         if (ret)
>                 return ret;
What do you think?



_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


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

end of thread, other threads:[~2022-03-21  9:47 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-15 13:39 [PATCH 1/2] of: implement new of_device_ensured_probed_by_alias_stem Andrej Picej
2022-03-15 13:39 ` [PATCH 2/2] mfd: da9063: ensure all gpio devices are probed before Andrej Picej
2022-03-15 14:24   ` Ahmad Fatoum
2022-03-16 10:44     ` Sascha Hauer
2022-03-21  9:46       ` Andrej Picej
2022-03-15 14:16 ` [PATCH 1/2] of: implement new of_device_ensured_probed_by_alias_stem Ahmad Fatoum
2022-03-15 14:29   ` Ahmad Fatoum
2022-03-16 10:01     ` Andrej Picej
2022-03-16  7:29   ` Andrej Picej

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