> 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 > > Reviewed-by: Simon Glass <s...@chromium.org> > > Nits below. > >> >> diff --git a/doc/device-tree-bindings/regulator/fan53555.txt >> b/doc/device-tree-bindings/regulator/fan53555.txt >> new file mode 100644 >> index 0000000..b183738 >> --- /dev/null >> +++ b/doc/device-tree-bindings/regulator/fan53555.txt >> @@ -0,0 +1,23 @@ >> +Binding for Fairchild FAN53555 regulators >> + >> +Required properties: >> + - compatible: "fcs,fan53555" >> + - reg: I2C address >> + >> +Optional properties: >> + - fcs,suspend-voltage-selector: declare which of the two available >> + voltage selector registers should be used for the suspend >> + voltage. The other one is used for the runtime voltage >> setting >> + Possible values are either <0> or <1> >> + - vin-supply: regulator supplying the vin pin >> + >> +Example: >> + >> + regulator@40 { >> + compatible = "fcs,fan53555"; >> + regulator-name = "fan53555"; >> + regulator-min-microvolt = <1000000>; >> + regulator-max-microvolt = <1800000>; >> + vin-supply = <&parent_reg>; >> + fcs,suspend-voltage-selector = <1>; >> + }; >> diff --git a/drivers/power/regulator/Kconfig >> b/drivers/power/regulator/Kconfig >> index 8892fa1..c26a765 100644 >> --- a/drivers/power/regulator/Kconfig >> +++ b/drivers/power/regulator/Kconfig >> @@ -69,6 +69,20 @@ config DM_REGULATOR_MAX77686 >> features for REGULATOR MAX77686. The driver implements get/set api >> for: >> value, enable and mode. >> >> +config DM_REGULATOR_FAN53555 >> + bool "Enable Driver Model for REGULATOR FAN53555" >> + depends on DM_REGULATOR && DM_I2C >> + ---help--- >> + This config enables implementation of driver-model regulator uclass >> + features for the FAN53555 regulator. The FAN53555 is a (family of) >> + single-output regulators that supports transitioning between two >> + different output voltages based on an voltage selection pin. >> + >> + The driver implements a get/set api for the voltage of the 'normal >> + mode' voltage only. Switching to 'suspend mode' (i.e. the alternate >> + voltage), disabling output via software, or switching the mode is >> + not supported by this driver (at this time). >> + >> config DM_REGULATOR_FIXED >> bool "Enable Driver Model for REGULATOR Fixed value" >> depends on DM_REGULATOR >> diff --git a/drivers/power/regulator/Makefile >> b/drivers/power/regulator/Makefile >> index 6c149a9..21040ea 100644 >> --- a/drivers/power/regulator/Makefile >> +++ b/drivers/power/regulator/Makefile >> @@ -11,6 +11,7 @@ obj-$(CONFIG_REGULATOR_AS3722) += as3722_regulator.o >> obj-$(CONFIG_DM_REGULATOR_MAX77686) += max77686.o >> obj-$(CONFIG_DM_REGULATOR_PFUZE100) += pfuze100.o >> obj-$(CONFIG_REGULATOR_PWM) += pwm_regulator.o >> +obj-$(CONFIG_$(SPL_)DM_REGULATOR_FAN53555) += fan53555.o >> obj-$(CONFIG_$(SPL_)DM_REGULATOR_FIXED) += fixed.o >> obj-$(CONFIG_$(SPL_)DM_REGULATOR_GPIO) += gpio-regulator.o >> obj-$(CONFIG_REGULATOR_RK8XX) += rk8xx.o >> diff --git a/drivers/power/regulator/fan53555.c >> b/drivers/power/regulator/fan53555.c >> new file mode 100644 >> index 0000000..3f0b4e9 >> --- /dev/null >> +++ b/drivers/power/regulator/fan53555.c >> @@ -0,0 +1,255 @@ >> +/* >> + * (C) 2017 Theobroma Systems Design und Consulting GmbH >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + */ >> + >> +#include <common.h> >> +#include <bitfield.h> >> +#include <errno.h> >> +#include <dm.h> >> +#include <fdtdec.h> >> +#include <i2c.h> >> +#include <asm/gpio.h> >> +#include <power/pmic.h> >> +#include <power/regulator.h> >> + >> +DECLARE_GLOBAL_DATA_PTR; >> + >> +/* >> + * The voltage ramp (i.e. minimum voltage and step) is defined by the >> + * combination of 2 nibbles: DIE_ID and DIE_REV. >> + * >> + * See http://www.onsemi.com/pub/Collateral/FAN53555-D.pdf for details. >> + */ > > struct comment? > >> +static const struct { >> + u8 die_id; >> + u8 die_rev; >> + u32 vsel_min; >> + u32 vsel_step; >> +} ic_types[] = { >> + { 0x0, 0x3, 600000, 10000 }, /* Option 00 */ >> + { 0x0, 0xf, 800000, 10000 }, /* Option 13 */ >> + { 0x0, 0xc, 600000, 12500 }, /* Option 23 */ >> + { 0x1, 0x3, 600000, 10000 }, /* Option 01 */ >> + { 0x3, 0x3, 600000, 10000 }, /* Option 03 */ >> + { 0x4, 0xf, 603000, 12826 }, /* Option 04 */ >> + { 0x5, 0x3, 600000, 10000 }, /* Option 05 */ >> + { 0x8, 0x1, 600000, 10000 }, /* Option 08 */ >> + { 0x8, 0xf, 600000, 10000 }, /* Option 08 */ >> + { 0xc, 0xf, 603000, 12826 }, /* Option 09 */ >> +}; >> + >> +/* I2C-accessible byte-sized registers */ >> +enum { >> + /* Voltage setting */ >> + FAN53555_VSEL0 = 0x00, >> + FAN53555_VSEL1, >> + /* Control register */ >> + FAN53555_CONTROL, >> + /* IC Type */ >> + FAN53555_ID1, >> + /* IC mask version */ >> + FAN53555_ID2, >> + /* Monitor register */ >> + FAN53555_MONITOR, >> +}; >> + >> +struct fan53555_platdata { >> + /* Voltage setting register */ >> + unsigned int vol_reg; >> + unsigned int sleep_reg; >> + >> +}; >> + >> +struct fan53555_priv { >> + /* IC Vendor */ >> + unsigned int vendor; >> + /* IC Type and Rev */ >> + unsigned int die_id; >> + unsigned int die_rev; >> + /* Voltage range and step(linear) */ >> + unsigned int vsel_min; >> + unsigned int vsel_step; >> + /* Voltage slew rate limiting */ >> + unsigned int slew_rate; >> + /* Sleep voltage cache */ >> + unsigned int sleep_vol_cache; >> +}; >> + >> +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... > >> +{ >> + int ret; >> + >> + ret = dm_i2c_write(dev, reg, buff, len); >> + if (ret) { >> + debug("%s: %s() failed to read reg %d\n", >> + dev->name, __func__, reg); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static int fan53555_read(struct udevice *dev, uint reg, u8 *buff, int len) > > pmic_reg_read()? > >> +{ >> + int ret; >> + >> + ret = dm_i2c_read(dev, reg, buff, len); >> + if (ret) { >> + debug("%s: %s() failed to read reg %d\n", >> + dev->name, __func__, reg); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static int fan53555_regulator_ofdata_to_platdata(struct udevice *dev) >> +{ >> + struct fan53555_platdata *dev_pdata = dev_get_platdata(dev); >> + struct dm_regulator_uclass_platdata *uc_pdata; >> + u32 sleep_vsel; >> + >> + uc_pdata = dev_get_uclass_platdata(dev); >> + if (!uc_pdata) >> + return -ENXIO; > > This cannot happen so this check is unnecessary, and I think it is > confusing too. > >> + >> + /* This is a buck regulator */ >> + uc_pdata->type = REGULATOR_TYPE_BUCK; >> + >> + sleep_vsel = dev_read_u32_default(dev, >> "fcs,suspend-voltage-selector", >> + FAN53555_VSEL1); >> + >> + /* >> + * Depending on the device-tree settings, the 'normal mode' >> + * voltage is either controlled by VSEL0 or VSEL1. >> + */ >> + switch (sleep_vsel) { >> + case FAN53555_VSEL0: >> + dev_pdata->sleep_reg = FAN53555_VSEL0; >> + dev_pdata->vol_reg = FAN53555_VSEL1; >> + break; >> + case FAN53555_VSEL1: >> + dev_pdata->sleep_reg = FAN53555_VSEL1; >> + dev_pdata->vol_reg = FAN53555_VSEL0; >> + break; >> + default: >> + pr_err("%s: invalid vsel id %d\n", dev->name, sleep_vsel); >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> +static int fan53555_regulator_get_value(struct udevice *dev) >> +{ >> + struct fan53555_platdata *pdata = dev_get_platdata(dev); >> + struct fan53555_priv *priv = dev_get_priv(dev); >> + u8 vol; >> + int voltage; >> + >> + /* We only support a single voltage selector (i.e. 'normal' mode). */ >> + fan53555_read(dev, pdata->vol_reg, &vol, 1); > > error check > >> + voltage = priv->vsel_min + (vol & 0x3f) * priv->vsel_step; >> + >> + debug("%s: %d uV\n", __func__, voltage); >> + return voltage; >> +} >> + >> +static int fan53555_regulator_set_value(struct udevice *dev, int uV) >> +{ >> + struct fan53555_platdata *pdata = dev_get_platdata(dev); >> + struct fan53555_priv *priv = dev_get_priv(dev); >> + u8 vol, oldbits, newbits; >> + >> + vol = (uV - priv->vsel_min) / priv->vsel_step; >> + fan53555_read(dev, pdata->vol_reg, &oldbits, 1); > > more error checks here and below > >> + newbits = bitfield_replace(oldbits, 0, 6, vol); >> + fan53555_write(dev, pdata->vol_reg, &newbits, 1); >> + >> + debug("%s: uV=%d; reg %d: %02x -> %02x\n", >> + __func__, uV, pdata->vol_reg, oldbits, newbits); >> + >> + return 0; >> +} >> + >> +static int fan53555_voltages_setup(struct udevice *dev) >> +{ >> + struct fan53555_priv *priv = dev_get_priv(dev); >> + struct dm_regulator_uclass_platdata *uc_pdata; >> + int i; >> + >> + uc_pdata = dev_get_uclass_platdata(dev); >> + if (!uc_pdata) >> + return -ENXIO; > > Again, please drop this check > >> + >> + /* Init voltage range and step */ >> + for (i = 0; i < ARRAY_SIZE(ic_types); ++i) { >> + if (ic_types[i].die_id != priv->die_id) >> + continue; >> + >> + if (ic_types[i].die_rev != priv->die_rev) >> + continue; >> + >> + priv->vsel_min = ic_types[i].vsel_min; >> + priv->vsel_step = ic_types[i].vsel_step; >> + >> + return 0; >> + } >> + >> + pr_err("%s: %s: die id %d rev %d not supported!\n", >> + dev->name, __func__, priv->die_id, priv->die_rev); >> + return -EINVAL; >> +} >> + >> +enum { >> + DIE_ID_SHIFT = 0, >> + DIE_ID_WIDTH = 4, >> + DIE_REV_SHIFT = 0, >> + DIE_REV_WIDTH = 4, >> +}; >> + >> + >> +static int fan53555_probe(struct udevice *dev) >> +{ >> + struct fan53555_priv *priv = dev_get_priv(dev); >> + u8 id1, id2; >> + >> + /* read chip id: vendor, die-id and die-revision */ >> + fan53555_read(dev, FAN53555_ID1, &id1, 1); >> + fan53555_read(dev, FAN53555_ID2, &id2, 1); >> + >> + priv->vendor = bitfield_extract(id1, 5, 3); >> + priv->die_id = id1 & GENMASK(3, 0); >> + priv->die_rev = id2 & GENMASK(3, 0); >> + >> + if (fan53555_voltages_setup(dev) < 0) >> + return -ENODATA; >> + >> + debug("%s: FAN53555 option %d rev %d detected\n", >> + __func__, priv->die_id, priv->die_rev); >> + >> + return 0; >> +} >> + >> +static const struct dm_regulator_ops fan53555_regulator_ops = { >> + .get_value = fan53555_regulator_get_value, >> + .set_value = fan53555_regulator_set_value, >> +}; >> + >> +static const struct udevice_id fan53555_regulator_ids[] = { >> + { .compatible = "fcs,fan53555" }, >> + { }, >> +}; >> + >> +U_BOOT_DRIVER(fan53555_regulator) = { >> + .name = "fan53555 regulator", >> + .id = UCLASS_REGULATOR, >> + .ops = &fan53555_regulator_ops, >> + .of_match = fan53555_regulator_ids, >> + .ofdata_to_platdata = fan53555_regulator_ofdata_to_platdata, >> + .platdata_auto_alloc_size = sizeof(struct fan53555_platdata), >> + .priv_auto_alloc_size = sizeof(struct fan53555_priv), >> + .probe = fan53555_probe, >> +}; >> -- >> 2.1.4 >> > > Regards, > Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot