Re: [PATCH 1/4] regulator: core: If consumers don't call regulator_set_load() assume max

[Doug Anderson]

Hi,

On Tue, Aug 14, 2018 at 11:30 AM, David Collins <collinsd@xxxxxxxxxxxxxx> wrote:
> The behavior introduced by this patch seems like an undesirable hack to
> me.  It goes against the general philosophy within the regulator framework
> of taking no action unless directed to do so by an explicit consumer
> request (or special device tree property).  We should assume that
> consumers make requests to meet their needs instead of assuming that they
> are missing important votes required by their hardware.

I don't agree so I guess it will be up to Mark to decide.

Specifically I will note that there are boatloads of drivers out there
that use the regulator framework but don't have a call to
regulator_set_load() in them.  Are these drivers all broken?  I don't
think so.  IMO the regulator_set_load() API is an optional call for
drivers that they can use to optimize power usage, not a required API.


>> --- a/drivers/regulator/core.c
>> +++ b/drivers/regulator/core.c
>> @@ -732,6 +732,7 @@ static int drms_uA_update(struct regulator_dev *rdev)
>>       struct regulator *sibling;
>>       int current_uA = 0, output_uV, input_uV, err;
>>       unsigned int mode;
>> +     bool any_unset = false;
>>
>>       lockdep_assert_held_once(&rdev->mutex);
>>
>> @@ -751,11 +752,17 @@ static int drms_uA_update(struct regulator_dev *rdev)
>>               return -EINVAL;
>>
>>       /* calc total requested load */
>> -     list_for_each_entry(sibling, &rdev->consumer_list, list)
>> +     list_for_each_entry(sibling, &rdev->consumer_list, list) {
>>               current_uA += sibling->uA_load;
>> +             if (!sibling->uA_load_set)
>> +                     any_unset = true;
>> +     }
>>
>>       current_uA += rdev->constraints->system_load;
>>
>> +     if (any_unset)
>> +             current_uA = INT_MAX;
>> +
>
> This check will incorrectly result in a constant load request of INT_MAX
> for all regulators that have at least one child regulator.  This is the
> case because such child regulators are present in rdev->consumer_list and
> because regulator_set_load() requests are not propagated up to parent
> regulators.  Thus, the regulator structs for child regulators will always
> have uA_load==0 and uA_load_set==false.

Ah, interesting.

...but doesn't this same bug exist anyway, just in the opposite
direction?  Without my patch we'll try to request a 0 mA load in this
case which seems just as wrong (or perhaps worse?).  I guess on RPMh
regulator you're "saved" because the RPMh hardware itself knows the
parent/child relationship and knows to ignore this 0 mA load, but it's
still a bug in the overall Linux framework...

I have no idea how we ought to propagate regulator_set_load() to
parents though.  That seems like a tough thing to try to handle
automagically.


-Doug