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

[Svyatoslav Ryhel]

5 серпня 2023 р. 23:46:27 GMT+03:00, Jonas Karlman <jonas at kwiboo.se> написав(-ла):
>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 :-)

So you have moved that, alright. Well, I am fine with less invasive patch as long as my devices continue to work as intended. I will test your suggestions and reload patchset. Thanks.

Best regards,
Svyatoslav R.

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