[U-Boot] [PATCH v2 1/2] power: regulator: add driver for the FAN53555 family
Dr. Philipp Tomsich
philipp.tomsich at theobroma-systems.com
Sun Nov 26 14:10:39 UTC 2017
> 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
>
> Reviewed-by: Simon Glass <sjg at 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 at 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
More information about the U-Boot
mailing list