Hi Philipp, On 27 November 2017 at 12:49, Dr. Philipp Tomsich <philipp.toms...@theobroma-systems.com> wrote: > >> On 27 Nov 2017, at 04:07, Simon Glass <s...@chromium.org> wrote: >> >> Hi Philipp, >> >> On 26 November 2017 at 07:10, Dr. Philipp Tomsich >> <philipp.toms...@theobroma-systems.com> wrote: >>> >>>> On 26 Nov 2017, at 12:38, Simon Glass <s...@chromium.org> wrote: >>>> >>>> Hi Philipp, >>>> >>>> On 22 November 2017 at 14:13, Philipp Tomsich >>>> <philipp.toms...@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.toms...@theobroma-systems.com> >>>>> Tested-by: Klaus Goger <klaus.go...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
