Re: [PATCH v3 1/6] regulator: core: validate selector against linear_min_sel

[Claudiu.Beznea]

Hi Jon,

On 24.11.2020 11:36, Jon Hunter wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 13/11/2020 15:21, Claudiu Beznea wrote:
>> There are regulators who's min selector is not zero. Selectors loops
>> (looping b/w zero and regulator::desc::n_voltages) might throw errors
>> because invalid selectors are used (lower than
>> regulator::desc::linear_min_sel). For this situations validate selectors
>> against regulator::desc::linear_min_sel.
> 
> 
> After this commit was merged, I noticed a regression in the DFLL (CPU
> clock source) on Tegra124. The DFLL driver
> (drivers/clk/tegra/clk-dfll.c) calls regulator_list_voltage() in a loop
> to determine the selector for a given voltage (see function
> find_vdd_map_entry_exact()).
> 
> Currently, the DFLL driver queries the number of voltages provided by
> the regulator by calling regulator_count_voltages() and then starting
> from 0, iterates through the number of voltages to find the selector
> value for the voltage it is looking for by calling
> regulator_list_voltage(). It assumes that any negative value returned by
> calling regulator_list_voltage() is an error and this will cause the
> loop up to terminate.
> 
> In this case the regulator in question is the as3722 and the
> linear_min_sel for this regulator is 1 and so when the DFLL driver calls
> regulator_list_voltage() with a selector value of 0 it now returns a
> negative error code, as expected by this change, and this terminates the
> loop up in the DFLL driver. So I can clearly see why this is happening
> and I could fix up the DFLL driver to avoid this.
> 
> Before doing so, I wanted to ask if that is the correct fix here,
> because it seems a bit odd that regulator_count_voltages() returns N
> voltages, but if the min selector value is greater than 0, then actually
> there are less than N. However, changing the number of voltages
> supported by the regulator to be N - linear_min_sel does not make sense
> either because then we need to know the linear_min_sel in order to
> determine the first valid voltage.
> 
> In case of the as3722, the value 0 means that the regulator is powered
> down. So it is a valid setting and equates to 0 volts at the output AFAICT.
> 
> Please let me know your thoughts are the correct way to fix this up.

I would say that a solution would be to have a new helper to retrieve the
linear_min_sel (e.g. regulator_min_sel()) and use this for all the
consumers of regulator_list_voltage() and the other APIs that patch
"regulator: core: validate selector against linear_min_sel" has changed
(regulator_list_voltage_table(), regulator_set_voltage_time()). With this
change the loop in find_vdd_map_entry_exact() should be b/w
regulator_min_sel() and regulator_count_voltages().

Maybe Mark has a better solution for this.

Thank you,
Claudiu

> 
> Thanks
> Jon
> 
> --
> nvpublic
>