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

[Svyatoslav Ryhel]

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.

>- 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.

>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
>