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
