Re: [U-Boot] [PATCH v3 06/17] dm: regulator: add implementation of driver model regulator uclass

Hi Przemyslaw,

On 24 March 2015 at 14:30, Przemyslaw Marczak p.marczak@samsung.com wrote:

This is the implementation of driver model regulator uclass api. To use it, the CONFIG_DM_PMIC is required with driver implementation, since it provides pmic devices basic I/O API.

To get the regulator device:

• regulator_get() - get the regulator device

The regulator framework is based on a 'struct dm_regulator_ops'. It provides a common function calls, for it's basic features:

• regulator_info() - get the regulator info structure
• regulator_mode() - get the regulator mode info structure
• regulator_get/set_value() - get/set the regulator output voltage
• regulator_get/set_current() - get/set the regulator output current
• regulator_get/set_enable() - get/set the regulator output enable state
• regulator_get/set_mode() - get/set the regulator output operation mode

An optional and useful regulator framework features are two descriptors:

• struct dm_regulator_info- describes the regulator name and output value limits

• struct dm_regulator_mode - (array) describes the regulators operation modes

The regulator framework features are described in file:

• include/power/regulator.h

Main files:

• drivers/power/regulator-uclass.c - provides regulator common functions api
• include/power/regulator.h - define all structures required by the regulator

Changes:

• new uclass-id: UCLASS_PMIC_REGULATOR
• new config: CONFIG_DM_REGULATOR

Signed-off-by: Przemyslaw Marczak p.marczak@samsung.com

Changes V2:

• new operations for regulator uclass:

-- get/set output state - for output on/off setting --- add enum: REGULATOR_OFF, REGULATOR_ON

• regulator uclass code rework and cleanup:

-- change name of: --- enum 'regulator_desc_type' to 'regulator_type' --- add type DVS --- struct 'regulator_desc' to 'regulator_value_desc'

-- regulator ops function calls: --- remove 'ldo/buck' from naming --- add new argument 'type' for define regulator type

-- regulator.h - update comments

Changes V3:

• regulator-uclass.c and regulator.h: -- api cleanup -- new function regulator_ofdata_to_platdata() -- update of comments -- add Kconfig

---

drivers/power/Kconfig | 33 ++++- drivers/power/Makefile | 1 + drivers/power/regulator-uclass.c | 219 +++++++++++++++++++++++++++++++++ include/dm/uclass-id.h | 1 + include/power/regulator.h | 259 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 512 insertions(+), 1 deletion(-) create mode 100644 drivers/power/regulator-uclass.c create mode 100644 include/power/regulator.h

diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig index 3513b46..1e73c7a 100644 --- a/drivers/power/Kconfig +++ b/drivers/power/Kconfig @@ -66,7 +66,38 @@ config DM_PMIC So the call will looks like below: 'pmic_write(regulator->parent, addr, value, len);'

-config AXP221_POWER +config DM_REGULATOR

Can you move this to drivers/power/regulator?

•   bool "Enable Driver Model for REGULATOR drivers (UCLASS_REGULATOR)"
  

•   depends on DM
  

•   ---help---
  

•   This config enables the driver-model regulator uclass support, which
  

•   provides implementation of driver model regulator uclass api.
  

•   Regulator uclass API calls:
  

•   To get the regulator device:
  

•   - regulator_get()             - get the regulator device
  

•   The regulator framework is based on a 'struct dm_regulator_ops'.
  

•   It provides a common function calls, for it's basic features:
  

•   - regulator_info()            - get the regulator info structure
  

•   - regulator_mode()            - get the regulator mode info structure
  

•   - regulator_get/set_value()   - operate on output voltage value
  

•   - regulator_get/set_current() - operate on output current value
  

•   - regulator_get/set_enable()  - operate on output enable state
  

•   - regulator_get/set_mode()    - operate on output operation mode
  

•   An optional and useful regulator framework features are two descriptors:
  

•   - struct dm_regulator_info - describes the regulator name and output limits
  

•   - struct dm_regulator_mode - describes the regulators operation mode
  

•   The regulator framework features are described in file:
  

•   - include/power/regulator.h
  

•   Main files:
  

•   - drivers/power/regulator-uclass.c - provides regulator common functions api
  

•   - include/power/regulator.h - define all structures required by the regulato
  

•   config AXP221_POWER
  

I don't think this should be indented.

    boolean "axp221 / axp223 pmic support"
    depends on MACH_SUN6I || MACH_SUN8I
    default y

diff --git a/drivers/power/Makefile b/drivers/power/Makefile index 5c9a189..a6b7012 100644 --- a/drivers/power/Makefile +++ b/drivers/power/Makefile @@ -22,3 +22,4 @@ obj-$(CONFIG_POWER_FSL) += power_fsl.o obj-$(CONFIG_POWER_I2C) += power_i2c.o obj-$(CONFIG_POWER_SPI) += power_spi.o obj-$(CONFIG_DM_PMIC) += pmic-uclass.o +obj-$(CONFIG_DM_REGULATOR) += regulator-uclass.o diff --git a/drivers/power/regulator-uclass.c b/drivers/power/regulator-uclass.c new file mode 100644 index 0000000..b6a00c6 --- /dev/null +++ b/drivers/power/regulator-uclass.c @@ -0,0 +1,219 @@ +/*

• • Copyright (C) 2014-2015 Samsung Electronics
• • Przemyslaw Marczak p.marczak@samsung.com
• • SPDX-License-Identifier: GPL-2.0+
• */

+#include <common.h> +#include <linux/types.h> +#include <fdtdec.h> +#include <dm.h> +#include <power/pmic.h> +#include <power/regulator.h> +#include <compiler.h> +#include <dm/device.h> +#include <dm/lists.h> +#include <dm/device-internal.h> +#include <errno.h>

+DECLARE_GLOBAL_DATA_PTR;

+int regulator_info(struct udevice *dev, struct dm_regulator_info **infop) +{

•   if (!dev || !dev->uclass_priv)
  

•           return -ENODEV;
  

assert() would be OK here, but if you prefer this, OK.

•   *infop = dev->uclass_priv;
  

•   return 0;
  

+}

+int regulator_mode(struct udevice *dev, struct dm_regulator_mode **modep) +{

•   struct dm_regulator_info *info;
  

•   int ret;
  

•   ret = regulator_info(dev, &info);
  

•   if (ret)
  

•           return ret;
  

•   *modep = info->mode;
  

•   return info->mode_count;
  

+}

+int regulator_get_value(struct udevice *dev) +{

•   const struct dm_regulator_ops *ops;
  

•   ops = pmic_get_uclass_ops(dev, UCLASS_REGULATOR);
  

Can you instead define regulator_get_uclass_ops()?

•   if (!ops)
  

•           return -ENODEV;
  

-ENOSYS

We normally assume that operations existing, so assert() is OK. But if you are worried about this, then this is OK.

•   if (!ops->get_value)
  

•           return -EPERM;
  

-ENOSYS

i.e.

if (!ops || !ops->get_value) return -ENOSYS;

Please fix below also.

•   return ops->get_value(dev);
  

+}

+int regulator_set_value(struct udevice *dev, int uV) +{

•   const struct dm_regulator_ops *ops;
  

•   ops = pmic_get_uclass_ops(dev, UCLASS_REGULATOR);
  

•   if (!ops)
  

•           return -ENODEV;
  

•   if (!ops->set_value)
  

•           return -EPERM;
  

•   return ops->set_value(dev, uV);
  

+}

+int regulator_get_current(struct udevice *dev) +{

•   const struct dm_regulator_ops *ops;
  

•   ops = pmic_get_uclass_ops(dev, UCLASS_REGULATOR);
  

•   if (!ops)
  

•           return -ENODEV;
  

•   if (!ops->get_current)
  

•           return -EPERM;
  

•   return ops->get_current(dev);
  

+}

+int regulator_set_current(struct udevice *dev, int uA) +{

•   const struct dm_regulator_ops *ops;
  

•   ops = pmic_get_uclass_ops(dev, UCLASS_REGULATOR);
  

•   if (!ops)
  

•           return -ENODEV;
  

•   if (!ops->set_current)
  

•           return -EPERM;
  

•   return ops->set_current(dev, uA);
  

+}

+bool regulator_get_enable(struct udevice *dev) +{

•   const struct dm_regulator_ops *ops;
  

•   ops = pmic_get_uclass_ops(dev, UCLASS_REGULATOR);
  

•   if (!ops)
  

•           return -ENODEV;
  

•   if (!ops->get_enable)
  

•           return -EPERM;
  

•   return ops->get_enable(dev);
  

+}

+int regulator_set_enable(struct udevice *dev, bool enable) +{

•   const struct dm_regulator_ops *ops;
  

•   ops = pmic_get_uclass_ops(dev, UCLASS_REGULATOR);
  

•   if (!ops)
  

•           return -ENODEV;
  

•   if (!ops->set_enable)
  

•           return -EPERM;
  

•   return ops->set_enable(dev, enable);
  

+}

+int regulator_get_mode(struct udevice *dev) +{

•   const struct dm_regulator_ops *ops;
  

•   ops = pmic_get_uclass_ops(dev, UCLASS_REGULATOR);
  

•   if (!ops)
  

•           return -ENODEV;
  

•   if (!ops->get_mode)
  

•           return -EPERM;
  

•   return ops->get_mode(dev);
  

+}

+int regulator_set_mode(struct udevice *dev, int mode) +{

•   const struct dm_regulator_ops *ops;
  

•   ops = pmic_get_uclass_ops(dev, UCLASS_REGULATOR);
  

•   if (!ops)
  

•           return -ENODEV;
  

•   if (!ops->set_mode)
  

•           return -EPERM;
  

•   return ops->set_mode(dev, mode);
  

+}

+int regulator_ofdata_to_platdata(struct udevice *dev) +{

•   struct dm_regulator_info *info = dev->uclass_priv;
  

•   int offset = dev->of_offset;
  

•   int len;
  

•   /* Mandatory constraints */
  

•   info->name = strdup(fdt_getprop(gd->fdt_blob, offset,
  

•                                   "regulator-name", &len));
  

•   if (!info->name)
  

•           return -ENXIO;
  

-ENOMEM

But there is no need to strdup() this - the device tree does not move.

•   info->min_uV = fdtdec_get_int(gd->fdt_blob, offset,
  

•                                 "regulator-min-microvolt", -1);
  

•   if (info->min_uV < 0)
  

•           return -ENXIO;
  

•   info->max_uV = fdtdec_get_int(gd->fdt_blob, offset,
  

•                                 "regulator-max-microvolt", -1);
  

•   if (info->max_uV < 0)
  

•           return -ENXIO;
  

•   /* Optional constraints */
  

•   info->min_uA = fdtdec_get_int(gd->fdt_blob, offset,
  

•                                 "regulator-min-microamp", -1);
  

•   info->max_uA = fdtdec_get_int(gd->fdt_blob, offset,
  

•                                 "regulator-max-microamp", -1);
  

•   info->always_on = fdtdec_get_bool(gd->fdt_blob, offset,
  

•                                    "regulator-always-on");
  

•   info->boot_on = fdtdec_get_bool(gd->fdt_blob, offset,
  

•                                    "regulator-boot-on");
  

•   return 0;
  

+}

+int regulator_get(char *name, struct udevice **devp) +{

•   struct dm_regulator_info *info;
  

•   struct udevice *dev;
  

•   int ret;
  

•   *devp = NULL;
  

•   for (ret = uclass_first_device(UCLASS_REGULATOR, &dev);
  

•        dev;
  

•        ret = uclass_next_device(&dev)) {
  

This will probe all devices. See my suggestion about creating uclass_find_first_device()/uclass_find_next_device() in the next patch.

As before, I think this could use a function like uclass_get_device_by_name().

•           info = dev->uclass_priv;
  

•           if (!info)
  

•                   continue;
  

•           if (!strcmp(name, info->name)) {
  

•                   *devp = dev;
  

•                   return ret;
  

•           }
  

•   }
  

•   return -ENODEV;
  

+}

+UCLASS_DRIVER(regulator) = {

•   .id             = UCLASS_REGULATOR,
  

•   .name           = "regulator",
  

•   .per_device_auto_alloc_size = sizeof(struct dm_regulator_info),
  

+}; diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index 3ecfa23..23356f3 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -37,6 +37,7 @@ enum uclass_id {

    /* PMIC uclass and PMIC-related uclass types */
    UCLASS_PMIC,

•   UCLASS_REGULATOR,
  

OK I see what the comment means now. I think it would be nice to have a comment here 'Voltage regulator'

    UCLASS_COUNT,
    UCLASS_INVALID = -1,

diff --git a/include/power/regulator.h b/include/power/regulator.h new file mode 100644 index 0000000..cf083c5 --- /dev/null +++ b/include/power/regulator.h @@ -0,0 +1,259 @@ +/*

• • Copyright (C) 2014-2015 Samsung Electronics
• • Przemyslaw Marczak p.marczak@samsung.com
• • SPDX-License-Identifier: GPL-2.0+
• */

+#ifndef _INCLUDE_REGULATOR_H_ +#define _INCLUDE_REGULATOR_H_

+/* enum regulator_type - used for regulator_*() variant calls */ +enum regulator_type {

•   REGULATOR_TYPE_LDO = 0,
  

•   REGULATOR_TYPE_BUCK,
  

•   REGULATOR_TYPE_DVS,
  

•   REGULATOR_TYPE_FIXED,
  

•   REGULATOR_TYPE_OTHER,
  

+};

+/**

• • struct dm_regulator_mode - this structure holds an information about
• • each regulator operation mode. Probably in most cases - an array.
• • This will be probably a driver-static data, since it is device-specific.
• • @id - a driver-specific mode id
• • @register_value - a driver-specific value for its mode id
• • @name - the name of mode - used for regulator command
• • Note:
• • The field 'id', should be always a positive number, since the negative values
• • are reserved for the errno numbers when returns the mode id.
• */

+struct dm_regulator_mode {

•   int id; /* Set only as >= 0 (negative value is reserved for errno) */
  

•   int register_value;
  

•   const char *name;
  

+};

+/**

• • struct dm_regulator_info - this structure holds an information about
• • each regulator constraints and supported operation modes. There is no "step"
• • voltage value - so driver should take care of this.
• • @type - one of 'enum regulator_type'
• • @mode - pointer to the regulator mode (array if more than one)
• • @mode_count - number of '.mode' entries
• • @min_uV* - minimum voltage (micro Volts)
• • @max_uV* - maximum voltage (micro Volts)
• • @min_uA* - minimum amperage (micro Amps)
• • @max_uA* - maximum amperage (micro Amps)
• • @always_on* - bool type, true or false
• • @boot_on* - bool type, true or false
• • @name* - fdt regulator name - should be taken from the device tree
• • Note: for attributes signed with '*'
• • These platform-specific constraints can be taken by regulator api function,
• • which is 'regulator_ofdata_to_platdata()'. Please read the description, which
• • can be found near the declaration of the mentioned function.

+*/ +struct dm_regulator_info {

•   enum regulator_type type;
  

•   struct dm_regulator_mode *mode;
  

•   int mode_count;
  

•   int min_uV;
  

•   int max_uV;
  

•   int min_uA;
  

•   int max_uA;
  

•   bool always_on;
  

•   bool boot_on;
  

•   const char *name;
  

+};

+/* PMIC regulator device operations */ +struct dm_regulator_ops {

•   /**
  

•    * The regulator output value function calls operates on a micro Volts.
  

•    *
  

•    * get/set_value - get/set output value of the given output number
  

•    * @dev          - regulator device
  

•    * Sets:
  

•    * @uV           - set the output value [micro Volts]
  

•    * Returns: output value [uV] on success or negative errno if fail.
  

•    */
  

•   int (*get_value)(struct udevice *dev);
  

•   int (*set_value)(struct udevice *dev, int uV);
  

•   /**
  

•    * The regulator output current function calls operates on a micro Amps.
  

•    *
  

•    * get/set_current - get/set output current of the given output number
  

•    * @dev            - regulator device
  

•    * Sets:
  

•    * @uA           - set the output current [micro Amps]
  

•    * Returns: output value [uA] on success or negative errno if fail.
  

•    */
  

•   int (*get_current)(struct udevice *dev);
  

•   int (*set_current)(struct udevice *dev, int uA);
  

•   /**
  

•    * The most basic feature of the regulator output is its enable state.
  

•    *
  

•    * get/set_enable - get/set enable state of the given output number
  

•    * @dev           - regulator device
  

•    * Sets:
  

•    * @enable         - set true - enable or false - disable
  

•    * Returns: true/false for get; or 0 / -errno for set.
  

•    */
  

•   bool (*get_enable)(struct udevice *dev);
  

•   int (*set_enable)(struct udevice *dev, bool enable);
  

•   /**
  

•    * The 'get/set_mode()' function calls should operate on a driver
  

driver-

•    * specific mode definitions, which should be found in:
  

•    * field 'mode' of struct mode_desc.
  

•    *
  

•    * get/set_mode - get/set operation mode of the given output number
  

•    * @dev         - regulator device
  

•    * Sets
  

•    * @mode_id     - set output mode id (struct dm_regulator_mode->id)
  

•    * Returns: id/0 for get/set on success or negative errno if fail.
  

•    * Note:
  

•    * The field 'id' of struct type 'dm_regulator_mode', should be always
  

•    * positive number, since the negative is reserved for the error.
  

•    */
  

•   int (*get_mode)(struct udevice *dev);
  

•   int (*set_mode)(struct udevice *dev, int mode_id);
  

+};

+/**

• • regulator_info: returns a pointer to the devices regulator info structure
• • @dev - pointer to the regulator device
• • @infop - pointer to the returned regulator info
• • Returns - 0 on success or negative value of errno.

@return 0 on success...

• */

+int regulator_info(struct udevice *dev, struct dm_regulator_info **infop);

+/**

• • regulator_mode: returns a pointer to the array of regulator mode info
• • @dev - pointer to the regulator device
• • @modep - pointer to the returned mode info array
• • Returns - count of modep entries on success or negative errno if fail.
• */

+int regulator_mode(struct udevice *dev, struct dm_regulator_mode **modep);

+/**

• • regulator_get_value: get microvoltage voltage value of a given regulator
• • @dev - pointer to the regulator device
• • Returns - positive output value [uV] on success or negative errno if fail.
• */

+int regulator_get_value(struct udevice *dev);

+/**

• • regulator_set_value: set the microvoltage value of a given regulator.
• • @dev - pointer to the regulator device
• • @uV - the output value to set [micro Volts]
• • Returns - 0 on success or -errno val if fails
• */

+int regulator_set_value(struct udevice *dev, int uV);

+/**

• • regulator_get_current: get microampere value of a given regulator
• • @dev - pointer to the regulator device
• • Returns - positive output current [uA] on success or negative errno if fail.
• */

+int regulator_get_current(struct udevice *dev);

+/**

• • regulator_set_current: set the microampere value of a given regulator.
• • @dev - pointer to the regulator device
• • @uA - set the output current [micro Amps]
• • Returns - 0 on success or -errno val if fails
• */

+int regulator_set_current(struct udevice *dev, int uA);

+/**

• • regulator_get_enable: get regulator device enable state.
• • @dev - pointer to the regulator device
• • Returns - true/false of enable state
• */

+bool regulator_get_enable(struct udevice *dev);

+/**

• • regulator_set_enable: set regulator enable state
• • @dev - pointer to the regulator device
• • @enable - set true or false
• • Returns - 0 on success or -errno val if fails
• */

+int regulator_set_enable(struct udevice *dev, bool enable);

+/**

• • regulator_get_mode: get mode of a given device regulator
• • @dev - pointer to the regulator device
• • Returns - positive mode number on success or -errno val if fails
• • Note:
• • The regulator driver should return one of defined, mode number rather, than
• • the raw register value. The struct type 'mode_desc' provides a field 'mode'
• • for this purpose and register_value for a raw register value.
• */

+int regulator_get_mode(struct udevice *dev);

+/**

• • regulator_set_mode: set given regulator mode
• • @dev - pointer to the regulator device
• • @mode - mode type (field 'mode' of struct mode_desc)
• • Returns - 0 on success or -errno value if fails
• • Note:
• • The regulator driver should take one of defined, mode number rather
• • than a raw register value. The struct type 'regulator_mode_desc' has
• • a mode field for this purpose and register_value for a raw register value.
• */

+int regulator_set_mode(struct udevice *dev, int mode);

+/**

• • regulator_ofdata_to_platdata: get the regulator constraints from its fdt node
• • and put it into the regulator 'dm_regulator_info' (dev->uclass_priv).
• • An example of required regulator fdt node constraints:
• • ldo1 {

• •  regulator-compatible = "LDO1"; (not used here, but required for bind)
    

• •  regulator-name = "VDD_MMC_1.8V"; (mandatory)
    

• •  regulator-min-microvolt = <1000000>; (mandatory)
    

• •  regulator-max-microvolt = <1000000>; (mandatory)
    

• •  regulator-min-microamp = <1000>; (optional)
    

• •  regulator-max-microamp = <1000>; (optional)
    

• •  regulator-always-on; (optional)
    

• •  regulator-boot-on; (optional)
    

• • };
• • @dev - pointer to the regulator device
• • Returns - 0 on success or -errno value if fails
• • Note2:
• • This function can be called at stage in which 'dev->uclass_priv' is non-NULL.
• • It is possible at two driver call stages: '.ofdata_to_platdata' or '.probe'.
• */

+int regulator_ofdata_to_platdata(struct udevice *dev);

+/**

• • regulator_get: returns the pointer to the pmic regulator device based on
• • regulator fdt node data
• • @fdt_name - property 'regulator-name' value of regulator fdt node
• • @devp - returned pointer to the regulator device
• • Returns: 0 on success or negative value of errno.
• • The returned 'regulator' device can be used with:
• • • regulator_get/set_*
• */

+int regulator_get(char *fdt_name, struct udevice **devp);

Can we s/fdt_name/name/ since by now it is a device name, not a device tree name.

+#endif /* _INCLUDE_REGULATOR_H_ */

1.9.1

Regards, Simon