[PATCH v2 3/7] power: regulator-uclass: perform regulator setup inside uclass

[Jonas Karlman]

On 2023-08-05 21:58, Svyatoslav Ryhel wrote:
> 
> 
> 5 серпня 2023 р. 15:49:32 GMT+03:00, Jonas Karlman <jonas at kwiboo.se> написав(-ла):
>> Hi,
>>
>> On 2023-07-21 07:34, Svyatoslav Ryhel wrote:
>>> чт, 20 лип. 2023 р. о 22:43 Simon Glass <sjg at chromium.org> пише:
>>>>
>>>> Hi Svyatoslav,
>>>>
>>>> On Thu, 20 Jul 2023 at 06:38, Svyatoslav Ryhel <clamor95 at gmail.com> wrote:
>>>>>
>>>>> Regulators initial setup was previously dependent on board call.
>>>>> To move from this behaviour were introduced two steps. First is
>>>>> that all individual regulators will be probed just after binding
>>>>
>>>> We must not probe devices automatically when bound. The i2c interface
>>>> may not be available, etc. Do a probe afterwards.
>>>>
>>>> Perhaps I am misunderstanding this, iwc please reword this commit message.
>>>
>>> After bound. If the regulator is a self-sufficient i2c device then it
>>> will be bound
>>> after i2c is available by code design so i2c interface should be
>>> available at that
>>> moment. At least led and gpio uclasses perform this for initial setup
>>> of devices.
>>>
>>> Platform regulators (aka fixed/gpio regulators) work perfectly fine. I
>>> have no i2c
>>> regulators to test deeply.
>>>
>>> As for now only one uclass is not compatible with this system - PMIC which has
>>> strong dependency between regulator and main mfd. This is why probing after
>>> bind for pmic regulators is disabled and pmic regulators are probed on pmic core
>>> post_probe.
>>
>> This sounds more like a possible bug of some dependency not being
>> probed in correct order or not leaving the device in a ready state
>> after probe.
>>
>> If pmic regulators are bind in pmic bind with the pmic as parent, then
>> at regulator probe the driver core ensure that pmic has probed before
>> regulator use.
>>
>> This works perfect fine with the RK8xx I2C PMIC and its regulators.
>> Wich a call to device_get_supply_regulator(dev, "test-supply", &reg)
>> probe happens in i2c, pmic and regulator order.
>>
> 
> I will check and re-test drivers I have ASAP. 
> 
>>>
>>>>> which ensures that regulator pdata is filled and second is setting
>>>>> up regulator in post probe which enseres that correct regulator
>>>>> state according to pdata is reached.
>>
>> I think that the approach in this patch is trying to change too much all
>> at once.
>>
>> Have made some adjustments that I think should help with transition.
>> - Added a flag to protect regulator_autoset from being called more then
>>  once for each regulator, in a separate patch.
> 
> It is not a good decision in the long run and you should keep in mind that there is nothing more constant than a temporary solution.

Nor do I propose this to be a long-term solution, only that it is more
reviewable to see changes in non-breaking smaller steps. In current
state this patch breaks building U-Boot and requires the last patch to
fix build again.

Anyway, I will be posting a similar patch for autoset as linked below as
part of a series to add a Rockchip IO-domain driver shortly. In current
state autoset cannot safely be called multiple times, and such patch
should not have an impact on the direction of this series.

> 
>> - Changed to only probe-after-bind on regulators marked as
>>  always-on/boot-on.
>>
>> And it works something like this:
>>
>> static int regulator_post_bind(struct udevice *dev)
>> {
>> 	[...]
>>
>> 	if (uc_pdata->always_on || uc_pdata->boot_on)
>> 		dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND);
>> }
>>
>> static int regulator_post_probe(struct udevice *dev)
>> {
>> 	ret = regulator_autoset(dev);
>> }
>>
>> With that any always-on/boot-on regulator would automatically probe and
>> autoset after bind, remaining would only probe and autoset if they are
>> used.
> 
> uc_pdata is filled in pre_probe, while you are runing check in post_bind.

Please look closer at the code and not the snippet above, they are
filled in post_bind or the code would not have worked :-)

Regards,
Jonas

> 
>> This approach should also prevent having to change existing code that
>> may call autoset, and cleanup can be done in follow-up patches/series.
>>
>> Probe-after-bind and call to autoset could also be protected with a new
>> Kconfig to help with transition.
>>
>> See following for a working example using a new driver that depends
>> on that regulators have been autoset prior to regulator_get_value.
>> https://github.com/Kwiboo/u-boot-rockchip/compare/master...rk3568-io-domain
>>
>> Or the two commits separate:
>>
>> power: regulator: Only run autoset once for each regulator
>> https://github.com/Kwiboo/u-boot-rockchip/commit/05db4dbcb8f908b417ed5cae8a7a325c82112d75
>>
>> power: regulator: Perform regulator setup inside uclass
>> https://github.com/Kwiboo/u-boot-rockchip/commit/489bd5d2c1a2a33824eac4f2d54399ef5dff4fdf
>>
>> Regards,
>> Jonas
>>
>>>>>
>>>>> Signed-off-by: Svyatoslav Ryhel <clamor95 at gmail.com>
>>>>> ---
>>>>>  drivers/power/regulator/regulator-uclass.c | 212 ++++++---------------
>>>>>  include/power/regulator.h                  | 119 ------------
>>>>>  2 files changed, 62 insertions(+), 269 deletions(-)
>>>>
>>>> Regards,
>>>> SImon
>>