[PATCH] power: regulator: Add support for regulator-force-boot-off

[Jaehoon Chung]

Hi Stefan,

On 4/9/21 2:20 PM, Stefan Roese wrote:
> Hi Jaehoon,
> 
> On 09.04.21 02:37, Jaehoon Chung wrote:
>> On 4/9/21 7:52 AM, Jaehoon Chung wrote:
>>> Hi Stefan,
>>>
>>> On 4/8/21 6:20 PM, Stefan Roese wrote:
>>>> From: Konstantin Porotchkin <kostap at marvell.com>
>>>>
>>>> Add support for regulator-force-boot-off DT property.
>>>> This property can be used by the board/device drivers for
>>>> turning off regulators on early init stages as pre-requisite
>>>> for the other components initialization.
>>>>
>>>> Signed-off-by: Konstantin Porotchkin <kostap at marvell.com>
>>>> Signed-off-by: Stefan Roese <sr at denx.de>
>>>> Cc: Jaehoon Chung <jh80.chung at samsung.com>
>>>> Cc: Simon Glass <sjg at chromium.org>
>>>> ---
>>>>   drivers/power/regulator/regulator-uclass.c | 38 ++++++++++++++++++++++
>>>>   include/power/regulator.h                  | 23 +++++++++++++
>>>>   2 files changed, 61 insertions(+)
>>>>
>>>> diff --git a/drivers/power/regulator/regulator-uclass.c b/drivers/power/regulator/regulator-uclass.c
>>>> index 4d2e730271f9..423867edced8 100644
>>>> --- a/drivers/power/regulator/regulator-uclass.c
>>>> +++ b/drivers/power/regulator/regulator-uclass.c
>>>> @@ -311,6 +311,17 @@ int regulator_autoset(struct udevice *dev)
>>>>       return ret;
>>>>   }
>>>>   +int regulator_unset(struct udevice *dev)
>>>> +{
>>>> +    struct dm_regulator_uclass_plat *uc_pdata;
>>>> +
>>>> +    uc_pdata = dev_get_uclass_plat(dev);
>>>> +    if (uc_pdata->force_off)
>>>
>>> Could you check whether uc_pdata is NULL or not?
>>> It can be returned to NULL.
>>>
>>>> +        return regulator_set_enable(dev, false);
>>>> +
>>>> +    return -EMEDIUMTYPE;
>>>> +}
>>>> +
>>>>   static void regulator_show(struct udevice *dev, int ret)
>>>>   {
>>>>       struct dm_regulator_uclass_plat *uc_pdata;
>>>> @@ -443,6 +454,7 @@ static int regulator_pre_probe(struct udevice *dev)
>>>>       uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");
>>>>       uc_pdata->ramp_delay = dev_read_u32_default(dev, "regulator-ramp-delay",
>>>>                               0);
>>>> +    uc_pdata->force_off = dev_read_bool(dev, "regulator-force-boot-off");
>>>>         node = dev_read_subnode(dev, "regulator-state-mem");
>>>>       if (ofnode_valid(node)) {
>>>> @@ -495,6 +507,32 @@ int regulators_enable_boot_on(bool verbose)
>>>>       return ret;
>>>>   }
>>>>   +int regulators_enable_boot_off(bool verbose)
>>>> +{
>>>> +    struct udevice *dev;
>>>> +    struct uclass *uc;
>>>> +    int ret;
>>>> +
>>>> +    ret = uclass_get(UCLASS_REGULATOR, &uc);
>>>> +    if (ret)
>>>> +        return ret;
>>>> +    for (uclass_first_device(UCLASS_REGULATOR, &dev);
>>>> +         dev;
>>>
>>> typo?
>>
>> I have checked that it was not typo. :)
>> how about making one line?
> 
> I could do this. But this would lead to a quite long line:
> 
>     for (uclass_first_device(UCLASS_REGULATOR, &dev); dev; uclass_next_device(&dev)) {
> 
> Only putting "dev;" into the same line is not appealing, at least not
> to me. Please note that this style of uclass looping is pretty
> common. It's also used a few lines above in regulators_enable_boot_on():
> 
> int regulators_enable_boot_on(bool verbose)
> {
>     struct udevice *dev;
>     struct uclass *uc;
>     int ret;
> 
>     ret = uclass_get(UCLASS_REGULATOR, &uc);
>     if (ret)
>         return ret;
>     for (uclass_first_device(UCLASS_REGULATOR, &dev);
>          dev;
>          uclass_next_device(&dev)) {
>         ret = regulator_autoset(dev);
> ...
> 
> So I would like to keep it as in v1.

Yes, If you prefer to it, keep it as in v1. 
I think that can be changed to below.

for (uclass_first_device(UCLASS_REGULATOR, &dev);
	dev; uclass_next_device(&dev)) {
	ret = reuglator_autoset(dev);
	...
}

But it's not critical thing. Thanks!

Best Regards,
Jaehoon Chung

> 
> Thanks,
> Stefan
> 
>>
>> Best Regards,
>> Jaehoo Chung
>>
>>>
>>> Best Regards,
>>> Jaehoon Chung
>>>
>>>> +         uclass_next_device(&dev)) {
>>>> +        ret = regulator_unset(dev);
>>>> +        if (ret == -EMEDIUMTYPE) {
>>>> +            ret = 0;
>>>> +            continue;
>>>> +        }
>>>> +        if (verbose)
>>>> +            regulator_show(dev, ret);
>>>> +        if (ret == -ENOSYS)
>>>> +            ret = 0;
>>>> +    }
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>>   UCLASS_DRIVER(regulator) = {
>>>>       .id        = UCLASS_REGULATOR,
>>>>       .name        = "regulator",
>>>> diff --git a/include/power/regulator.h b/include/power/regulator.h
>>>> index da9a065bdde0..fad87c99e5db 100644
>>>> --- a/include/power/regulator.h
>>>> +++ b/include/power/regulator.h
>>>> @@ -151,6 +151,7 @@ enum regulator_flag {
>>>>    * @max_uA*    - maximum amperage (micro Amps)
>>>>    * @always_on* - bool type, true or false
>>>>    * @boot_on*   - bool type, true or false
>>>> + * @force_off* - bool type, true or false
>>>>    * TODO(sjg at chromium.org): Consider putting the above two into @flags
>>>>    * @ramp_delay - Time to settle down after voltage change (unit: uV/us)
>>>>    * @flags:     - flags value (see REGULATOR_FLAG_...)
>>>> @@ -176,6 +177,7 @@ struct dm_regulator_uclass_plat {
>>>>       unsigned int ramp_delay;
>>>>       bool always_on;
>>>>       bool boot_on;
>>>> +    bool force_off;
>>>>       const char *name;
>>>>       int flags;
>>>>       u8 ctrl_reg;
>>>> @@ -420,6 +422,15 @@ int regulator_set_mode(struct udevice *dev, int mode_id);
>>>>    */
>>>>   int regulators_enable_boot_on(bool verbose);
>>>>   +/**
>>>> + * regulators_enable_boot_off() - disable regulators needed for boot
>>>> + *
>>>> + * This disables all regulators which are marked to be off at boot time.
>>>> + *
>>>> + * This effectively calls regulator_unset() for every regulator.
>>>> + */
>>>> +int regulators_enable_boot_off(bool verbose);
>>>> +
>>>>   /**
>>>>    * regulator_autoset: setup the voltage/current on a regulator
>>>>    *
>>>> @@ -439,6 +450,18 @@ int regulators_enable_boot_on(bool verbose);
>>>>    */
>>>>   int regulator_autoset(struct udevice *dev);
>>>>   +/**
>>>> + * regulator_unset: turn off a regulator
>>>> + *
>>>> + * The setup depends on constraints found in device's uclass's platform data
>>>> + * (struct dm_regulator_uclass_platdata):
>>>> + *
>>>> + * - Disable - will set - if  'force_off' is set to true,
>>>> + *
>>>> + * The function returns on the first-encountered error.
>>>> + */
>>>> +int regulator_unset(struct udevice *dev);
>>>> +
>>>>   /**
>>>>    * regulator_autoset_by_name: setup the regulator given by its uclass's
>>>>    * platform data name field. The setup depends on constraints found in device's
>>>>
>>>
>>>
>>
> 
> 
> Viele Grüße,
> Stefan
>