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

[Dr. Philipp Tomsich]

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

Cheers,
Philipp.