Re: [PATCH 3/5] regulator: allow use of dummy regulator

[Andrej Picej <andrej.picej@norik.com>]

On 17. 11. 21 09:11, Andrej Picej wrote:
> 
> 
> On 17. 11. 21 08:21, Sascha Hauer wrote:
>> On Mon, Nov 15, 2021 at 02:02:06PM +0100, Andrej Picej wrote:
>>> It is quite common for users to delete power supply nodes of regulators
>>> which aren't yet supported.
>>> The idea of a function call or devicetree property which allows use of
>>> dummy regulator is not new. This implementation uses barebox specific
>>> devicetree property "barebox,allow-dummy-supply" to allow switching to
>>> dummy power regulator.
>>> Basically just catch the regulators ensure_probed error, if this
>>> property is set.
>>>
>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>>> ---
>>>   drivers/regulator/core.c | 9 ++++++++-
>>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
>>> index 097f7d779..1c58932e1 100644
>>> --- a/drivers/regulator/core.c
>>> +++ b/drivers/regulator/core.c
>>> @@ -231,8 +231,15 @@ static struct regulator_internal 
>>> *of_regulator_get(struct device_d *dev, const c
>>>       }
>>>       ret = of_device_ensure_probed(node);
>>> -    if (ret)
>>> +    if (ret) {
>>> +        if (of_get_property(dev->device_node, 
>>> "barebox,allow-dummy-supply", NULL)) {
>>> +            dev_dbg(dev, "Allow use of dummy regulator for " \
>>> +                "%s-supply\n", supply);
>>> +            ri = NULL;
>>> +            goto out;
>>> +        }
>>>           return ERR_PTR(ret);
>>
>> I wonder if we should rather add a property on the producer side than on
>> the consumer side, i.e. Add a barebox,status = "disabled" property to
>> the regulator node. We had the same discussion with phys recently, maybe
>> we can use the same approach for both issues.
>>
> 
> I was wandering that too. But decided to go with consumer side so users 
> which might want to use this have to enable this for every consumers 
> which can use dummy regulators. IMO this would mean more thought would 
> go into this and would be consequently more error prone.
> 
> So for producer side did you have in mind that this setting will be set 
> for every regulator, like this?
> 
>>                 regulators {
>>                         vddcore_reg: bcore1 {
>> +                               barebox,allow-dummy-supply;
>>                                 regulator-min-microvolt = <730000>;
>>                                 regulator-max-microvolt = <1380000>;
>>                                 regulator-always-on;
>>                         };
> 
> Or should this setting be set for all regulators provided by the same 
> IC, like this?
> 
>>                 regulators {
>> +                       barebox,allow-dummy-supply;
>>                         vddcore_reg: bcore1 {
>>                                 regulator-min-microvolt = <730000>;
>>                                 regulator-max-microvolt = <1380000>;
>>                                 regulator-always-on;
>>                         };
> 
> I guess we could make both cases work, first check if 
> "barebox,allow-dummy-supply" is present in the regulator node and then 
> also check parent node (regulators)? What do you think?
> 

Like this:

> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 097f7d779..4ff7e1c2f 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -197,7 +197,7 @@ static struct regulator_internal *of_regulator_get(struct device_d *dev, const c
>  {
>         char *propname;
>         struct regulator_internal *ri;
> -       struct device_node *node;
> +       struct device_node *node, *node_parent;
>         int ret;
>  
>         propname = basprintf("%s-supply", supply);
> @@ -231,8 +231,23 @@ static struct regulator_internal *of_regulator_get(struct device_d *dev, const c
>         }
>  
>         ret = of_device_ensure_probed(node);
> -       if (ret)
> +       if (ret) {
> +               /* If "barebox,allow-dummy-supply" property is set for regulator
> +                * provider, allow use of dummy regulator -> NULL is returned.
> +                * Check regulator node and its parent, if this setting is set
> +                * PMIC wide.
> +                */
> +               node_parent = of_get_parent(node);
> +               if (of_get_property(node, "barebox,allow-dummy-supply", NULL) ||
> +                   of_get_property(node_parent, "barebox,allow-dummy-supply", NULL)) {
> +                       dev_dbg(dev, "Allow use of dummy regulator for " \
> +                               "%s-supply\n", supply);
> +                       ri = NULL;
> +                       goto out;
> +               }
> +
>                 return ERR_PTR(ret);
> +       }
>  
>         list_for_each_entry(ri, &regulator_list, list) {
>                 if (ri->node == node) {

BR,
Andrej

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