[U-Boot] [PATCH] power: pmic/regulator: Add basic support for TPS65910

[Simon Glass]

Hi Felix,

On 27 November 2017 at 06:51, Felix Brack <fb at ltec.ch> wrote:
> Hello Simon,
>
> On 26.11.2017 12:38, Simon Glass wrote:
>> Hi Felix,
>>
>> On 23 November 2017 at 08:36, Felix Brack <fb at ltec.ch> wrote:
>>>
>>>
>>> On 22.11.2017 11:39, Felix Brack wrote:
>>>> Hello Simon,
>>>>
>>>> Many thanks for taking the time to review my patch.
>>>>
>>>> On 20.11.2017 16:38, Simon Glass wrote:
>>>>> Hi Felix,
>>>>>
>>>>>> +
>>>>>> +/* platform data */
>>>>>> +struct tps65910_pdata {
>>>>>> +       u32 supply[TPS65910_NUM_SUPPLIES]; /* regulator supply voltage in uV */
>>>>>> +};
>>>>>
>>>>> Is this used outside the driver? Do you need one driver to access
>>>>> another's platform data? That seems dodgy.
>>>>>
>>>> No. You are right: no need to place it in the header file. I will move
>>>> it to pmic_tps65910_dm.c file.
>>>>
>>> Sorry, this is not correct. The regulators of the PMIC need access to
>>> this. Let me try to explain:
>>>
>>> The PMIC has 8 different supply inputs. These are therefore properties
>>> of the PMIC. Each of the 12 regulators uses one of these 8 supply
>>> voltages as its input. Hence the regulators of the TPS65910 need read
>>> access to the PMIC's information about the supplies. In other words: a
>>> regulator's supply voltage has to be queried from the corresponding PMIC.
>>>
>>> I think I followed the "rules" when implementing the PMIC and the
>>> regulator code in separate files.
>>
>> OK, so the problem is that the regulator has to look up the supply
>> voltage in the PMIC?
>>
> Yes
>
>> Should this be platform data (accessible before the device is probed)
>> or private data (which exists only when the device is active)?
>>
> The regulator supply voltages are defined in the DT. Hence they should
> be read by ofdata_to_platdata() and store as platform data, I think.

OK.

>
>> Also why does the PMIC driver itself know about supply voltages? I
>> think that info should be in the regulator driver. The PMIC is really
>> just a parent driver providing access to the various subsystems of the
>> PMIC (regulator, GPIO, battery, etc).
>>
> I agree - the PMIC driver does not require the knowledge about the
> supply voltage of a distinct regulator. It is just that the PMIC is
> modeled like this in the DT, i.e. the supplies are proerties of the PMIC
> and not the regulators (which are subnodes on the same level as the
> supply nodes).
> Having said that, I think the proper solution is as follows:
> Use the regulator driver's ofdata_to_platdata() to query the supply
> voltage of that specific regulator by device_get_supply_regulator()
> using dev->parent (the PMIC) as device parameter.
> The supply voltage will then be "part" of the regulator instead of the
> PMIC. Do you agree to this solution?

Yes I think that is best from what I understand.

However if you find that you really do need to blur the line between
the regulator and the pmic, and share the pdata structure, it is not
the end of the world. It seems wrong to me, so I'd like to avoid it if
possible. We should have a good reason. The DT node structure is
certainly a reason, but it does not seem a strong enough reason to me.

Regards,
Simon