On 2023-08-05 21:58, Svyatoslav Ryhel wrote: > > > 5 серпня 2023 р. 15:49:32 GMT+03:00, Jonas Karlman <jo...@kwiboo.se> > написав(-ла): >> Hi, >> >> On 2023-07-21 07:34, Svyatoslav Ryhel wrote: >>> чт, 20 лип. 2023 р. о 22:43 Simon Glass <s...@chromium.org> пише: >>>> >>>> Hi Svyatoslav, >>>> >>>> On Thu, 20 Jul 2023 at 06:38, Svyatoslav Ryhel <clamo...@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", ®) >> 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 <clamo...@gmail.com> >>>>> --- >>>>> drivers/power/regulator/regulator-uclass.c | 212 ++++++--------------- >>>>> include/power/regulator.h | 119 ------------ >>>>> 2 files changed, 62 insertions(+), 269 deletions(-) >>>> >>>> Regards, >>>> SImon >>