[U-Boot] [PATCH v2 1/2] power: regulator: add driver for the FAN53555 family

[Simon Glass]

Hi Philipp,

On 27 November 2017 at 12:49, Dr. Philipp Tomsich
<philipp.tomsich at theobroma-systems.com> wrote:
>
>> On 27 Nov 2017, at 04:07, Simon Glass <sjg at chromium.org> wrote:
>>
>> Hi Philipp,
>>
>> On 26 November 2017 at 07:10, Dr. Philipp Tomsich
>> <philipp.tomsich at theobroma-systems.com> wrote:
>>>
>>>> On 26 Nov 2017, at 12:38, Simon Glass <sjg at chromium.org> wrote:
>>>>
>>>> Hi Philipp,
>>>>
>>>> On 22 November 2017 at 14:13, Philipp Tomsich
>>>> <philipp.tomsich at theobroma-systems.com> wrote:
>>>>> This adds a driver for the FAN53555 family of regulators.
>>>>>
>>>>> While these devices support a 'normal' and 'suspend' mode (controlled
>>>>> via an external pin) to switch between two programmable voltages, this
>>>>> incarnation of the driver assumes that the device is always operating
>>>>> in 'normal' mode.
>>>>>
>>>>> Only setting/reading the programmed voltage is supported at this time
>>>>> and the following device functionality remains unsupported:
>>>>> - switching the selected voltage (via a GPIO)
>>>>> - disabling the voltage output via software-control
>>>>> This matches the functionality of the Linux driver.
>>>>>
>>>>> Tested on a RK3399-Q7 (with 'option 5' devices): setting voltages from
>>>>> the U-Boot shell and verifying output voltages on the board.
>>>>>
>>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich at theobroma-systems.com>
>>>>> Tested-by: Klaus Goger <klaus.goger at theobroma-systems.com>
>>>>>
>>>>> ---
>>>>>
>>>>> Changes in v2:
>>>>> - adapted documentation on the device-tree binding from Linux
>>>>>
>>>>> doc/device-tree-bindings/regulator/fan53555.txt |  23 +++
>>>>> drivers/power/regulator/Kconfig                 |  14 ++
>>>>> drivers/power/regulator/Makefile                |   1 +
>>>>> drivers/power/regulator/fan53555.c              | 255 ++++++++++++++++++++++++
>>>>> 4 files changed, 293 insertions(+)
>>>>> create mode 100644 doc/device-tree-bindings/regulator/fan53555.txt
>>>>> create mode 100644 drivers/power/regulator/fan53555.c
>> [...]
>>
>>>>> +static int fan53555_write(struct udevice *dev, uint reg, u8 *buff, int len)
>>>>
>>>> In this file en is only ever 1. How about using pmic_reg_write()?
>>>
>>> pmic_reg_write would require the regulator to be part of a PMIC
>>> device (i.e. have the pmic as a parent).  This is a pure regulator
>>> that is not part of a PMIC.
>>>
>>> If the intent is to not have such devices, I can model this as a
>>> PMIC with a single regulator...
>>
>> Yes I *think* all regulators should have a PMIC as a parent. A PMIC is
>> a type of multi-function device.
>
> I disagree, although the fan53555 (if we ever fully implement support for
> it beyond what Linux supports) might fall into a grey area.
>
> Let’s say, I add the intermediate PMIC (which is a quick change and I was
> tempted to do it instead of keeping this discussion alive, but I think this is
> worthwhile to be discussed a bit further):
> 1.      The DM tree would not correspond to the DTS, as the regulator is
>         modelled below the I2C-device for the FAN53555 and (if we assume
>         this node becomes the PMIC) there’s no regulators below that.
> 2.      Consequently, I’d need to implement a custom bind() method for this
>         PMIC just to bind a single regulator below it.
>
> This sounds very wasteful, as the only thing I’d gain would be the use of
> the pmic_reg_*() family of functions.  Admittedly, I’d like to to have use of
> this IO-abstraction, but it still seems a bit wasteful to do this.
>
> Now, let’s assume we ever implement the advanced functions of the
> regulator, which adds switching between two set voltages (routed to a
> single output); this would give two options of modelling it (if there was
> a PMIC device in the hierarchy):
> A.      create two children for VSEL1 and VSEL2 and provide a set-voltage
>         method for each… and not associate the GPIO for switching between
>         these with any of them
> B.      use the regulator framework (and manage the GPIO as part of the
>         set-mode verb/action) … and have a single child below the PMIC
>         again.
>
> Now, in that case, I’d go with option B.
>
> So in summary: we’ll always end up with a single regulator below a
> PMIC that will need to provide a custom bind-op… but I’d be able to
> reuse the pmic_reg_* functions.
>
> I am not convinced yet, but I’ll go with whatever we decide here.

Long delay, but I found this email again.

At present most regulators have a PMIC as a parent, but I don't see
anywhere where it is required. Perhaps we should document what should
be done here?

Regards,
Simon