Re: [U-Boot] [PATCH v2 04/12] dm: pmic: add implementation of driver model regulator uclass

Hello Simon,

On 03/06/2015 03:12 PM, Simon Glass wrote:

Hi Przemyslaw,

On 3 March 2015 at 09:24, 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.

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

• regulator_get_cnt() - number of outputs each type
• regulator_get_value_desc() - describe output value limits
• regulator_get_mode_desc() - describe output operation modes
• regulator_get/set_value() - output value (uV)
• regulator_get/set_state() - output on/off state

Would get/set_enable() be better?

...

• regulator_get/set_mode() - output operation mode

To get the regulator device:

• regulator_get() - by name only
• regulator_i2c_get() - by i2c bus address (of pmic parent)
• regulator_spi_get() - by spi bus address (of pmic parent)

For the last two, why are we using bus address? This should be by a phandle reference or nane.

...

An optional and useful regulator framework features are two descriptors:

• struct regulator_desc - describes the regulator name and output value limits should be defined by device driver for each regulator output.

• struct regulator_mode_desc - (array) describes a number of operation modes supported by each regulator output.

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

drivers/power/Makefile | 1 + drivers/power/regulator-uclass.c | 227 ++++++++++++++++++++++++++++ include/dm/uclass-id.h | 1 + include/power/regulator.h | 310 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 539 insertions(+) create mode 100644 drivers/power/regulator-uclass.c create mode 100644 include/power/regulator.h

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..6b5c678 --- /dev/null +++ b/drivers/power/regulator-uclass.c @@ -0,0 +1,227 @@ +/*

• • 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_get_cnt(struct udevice *dev, int type, int *cnt) +{

•   const struct dm_regulator_ops *ops;
  

•   ops = pmic_get_uclass_ops(dev, UCLASS_PMIC_REGULATOR);
  

•   if (!ops)
  

•           return -ENODEV;
  

This is an error so you should be able to just assert(). Failing that let's use -ENXIO as you do below.

...

•   if (!ops->get_cnt)
  

•           return -EPERM;
  

-ENOSYS for these.

Ah, I missed this one, will fix in V4.

...

•   return ops->get_cnt(dev, type, cnt);
  

+}

+int regulator_get_value_desc(struct udevice *dev, int type, int number,

•                        struct regulator_value_desc **desc)
  

+{

•   const struct dm_regulator_ops *ops;
  

•   ops = pmic_get_uclass_ops(dev, UCLASS_PMIC_REGULATOR);
  

•   if (!ops)
  

•           return -ENXIO;
  

•   if (!ops->get_value_desc)
  

•           return -EPERM;
  

•   return ops->get_value_desc(dev, type, number, desc);
  

+}

+int regulator_get_mode_desc(struct udevice *dev, int type, int number,

•                       int *mode_cnt, struct regulator_mode_desc **desc)
  

+{

•   const struct dm_regulator_ops *ops;
  

•   ops = pmic_get_uclass_ops(dev, UCLASS_PMIC_REGULATOR);
  

•   if (!ops)
  

•           return -ENXIO;
  

•   if (!ops->get_mode_desc_array)
  

•           return -EPERM;
  

•   return ops->get_mode_desc_array(dev, type, number, mode_cnt, desc);
  

+}

+int regulator_get_value(struct udevice *dev, int type, int number, int *value) +{

•   const struct dm_regulator_ops *ops;
  

•   ops = pmic_get_uclass_ops(dev, UCLASS_PMIC_REGULATOR);
  

•   if (!ops)
  

•           return -ENODEV;
  

•   if (!ops->get_value)
  

•           return -EPERM;
  

•   return ops->get_value(dev, type, number, value);
  

+}

+int regulator_set_value(struct udevice *dev, int type, int number, int value) +{

•   const struct dm_regulator_ops *ops;
  

•   ops = pmic_get_uclass_ops(dev, UCLASS_PMIC_REGULATOR);
  

•   if (!ops)
  

•           return -ENODEV;
  

•   if (!ops->set_value)
  

•           return -EPERM;
  

•   return ops->set_value(dev, type, number, value);
  

+}

+int regulator_get_state(struct udevice *dev, int type, int number, int *state) +{

•   const struct dm_regulator_ops *ops;
  

•   ops = pmic_get_uclass_ops(dev, UCLASS_PMIC_REGULATOR);
  

•   if (!ops)
  

•           return -ENODEV;
  

•   if (!ops->get_state)
  

•           return -EPERM;
  

•   return ops->get_state(dev, type, number, state);
  

+}

+int regulator_set_state(struct udevice *dev, int type, int number, int state) +{

•   const struct dm_regulator_ops *ops;
  

•   ops = pmic_get_uclass_ops(dev, UCLASS_PMIC_REGULATOR);
  

•   if (!ops)
  

•           return -ENODEV;
  

•   if (!ops->set_state)
  

•           return -EPERM;
  

•   return ops->set_state(dev, type, number, state);
  

+}

+int regulator_get_mode(struct udevice *dev, int type, int number, int *mode) +{

•   const struct dm_regulator_ops *ops;
  

•   ops = pmic_get_uclass_ops(dev, UCLASS_PMIC_REGULATOR);
  

•   if (!ops)
  

•           return -ENODEV;
  

•   if (!ops->get_mode)
  

•           return -EPERM;
  

•   return ops->get_mode(dev, type, number, mode);
  

+}

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

•   const struct dm_regulator_ops *ops;
  

•   ops = pmic_get_uclass_ops(dev, UCLASS_PMIC_REGULATOR);
  

•   if (!ops)
  

•           return -ENODEV;
  

•   if (!ops->set_mode)
  

•           return -EPERM;
  

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

+}

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

•   struct udevice *dev;
  

•   struct uclass *uc;
  

•   int ret;
  

•   ret = uclass_get(UCLASS_PMIC_REGULATOR, &uc);
  

•   if (ret) {
  

•           error("Regulator uclass not initialized!");
  

•           return ret;
  

•   }
  

•   uclass_foreach_dev(dev, uc) {
  

•           if (!dev)
  

•                   continue;
  

•           if (!strncmp(name, dev->name, strlen(name))) {
  

•                   ret = device_probe(dev);
  

•                   if (ret)
  

•                           error("Dev: %s probe failed", dev->name);
  

•                   *regulator = dev;
  

•                   return ret;
  

•           }
  

•   }
  

•   *regulator = NULL;
  

•   return -EINVAL;
  

+}

+int regulator_i2c_get(int bus, int addr, struct udevice **regulator) +{

•   struct udevice *pmic;
  

•   int ret;
  

•   /* First get parent pmic device */
  

•   ret = pmic_i2c_get(bus, addr, &pmic);
  

•   if (ret) {
  

•           error("Chip: %d on bus %d not found!", addr, bus);
  

•           return ret;
  

•   }
  

•   /* Get the first regulator child of pmic device */
  

•   if (device_get_first_child_by_uclass_id(pmic, UCLASS_PMIC_REGULATOR,
  

•                                           regulator)) {
  

•           error("PMIC: %s regulator child not found!", pmic->name);
  

•           *regulator = NULL;
  

•           return ret;
  

•   }
  

•   return 0;
  

+}

+int regulator_spi_get(int bus, int cs, struct udevice **regulator) +{

•   struct udevice *pmic;
  

•   int ret;
  

•   /* First get parent pmic device */
  

•   ret = pmic_spi_get(bus, cs, &pmic);
  

•   if (ret) {
  

•           error("Chip: %d on bus %d not found!", cs, bus);
  

•           return ret;
  

•   }
  

•   /* Get the first regulator child of pmic device */
  

•   if (device_get_first_child_by_uclass_id(pmic, UCLASS_PMIC_REGULATOR,
  

•                                           regulator)) {
  

•           error("PMIC: %s regulator child not found!", pmic->name);
  

•           *regulator = NULL;
  

•           return ret;
  

•   }
  

•   return 0;
  

+}

+UCLASS_DRIVER(regulator) = {

•   .id             = UCLASS_PMIC_REGULATOR,
  

•   .name           = "regulator",
  

+}; diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index ff360ac..0a1e664 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 uclasses */
     UCLASS_PMIC,

•   UCLASS_PMIC_REGULATOR,
  
     UCLASS_COUNT,
     UCLASS_INVALID = -1,
  

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

• • 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_

+#define DESC_NAME_LEN 20

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

•   REGULATOR_TYPE_LDO = 0,
  

•   REGULATOR_TYPE_BUCK,
  

•   REGULATOR_TYPE_DVS,
  

+};

+/* enum regulator_out_state - used for ldo/buck output state setting */ +enum regulator_out_state {

•   REGULATOR_OFF = 0,
  

•   REGULATOR_ON,
  

+};

If we will only ever have on/off perhaps we don't need this enum and could just use a bool?

Fixed in V3.

...

+/**

• • struct regulator_value_desc - this structure holds an information about
• • each regulator voltage limits. There is no "step" voltage value - so driver
• • should take care of this. This is rather platform specific so can be
• • taken from the device-tree.
• • @type - one of: DESC_TYPE_LDO, DESC_TYPE_BUCK or DESC_TYPE_DVS
• • @number - hardware internal regulator number

What is this numbering and where does it come from? It is specified by the PMIC datasheet? Can you expand the comment a bit?

Actually I think this numbering should go away and we should use names instead.

This suggestion is implemented in V3.

...

• • @min_uV - minimum voltage (micro Volts)
• • @max_uV - maximum voltage (micro Volts)
• • @name - name of regulator - usually will be taken from device tree and

• •       can describe regulator output connected hardware, e.g. "eMMC"
    

+*/ +struct regulator_value_desc {

•   int type;
  

•   int number;
  

•   int min_uV;
  

•   int max_uV;
  

•   char name[DESC_NAME_LEN];
  

What do you think about making this a const char * and we alloc it with strdup()?

Also implemented in V3.

...

+};

+/**

• • struct regulator_mode_desc - this structure holds an information about
• • each regulator operation modes. Probably in most cases - an array.
• • This will be probably a driver static data since it's device specific.

driver-static (I think you mean)

...

• • @mode - a driver specific mode number - used for regulator command
• • @register_value - a driver specific value for this mode

driver-specific, you are creating an adjective

...

• • @name - the name of mode - used for regulator command
• */

+struct regulator_mode_desc {

•   int mode;
  

•   int register_value;
  

•   char *name;
  

+};

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

•   /**
  

•    * get_cnt - get the number of a given regulator 'type' outputs
  

•    *
  

•    * @dev    - regulator device
  

•    * @type   - regulator type, one of enum regulator_type
  

•    * Gets:
  

•    * @cnt    - regulator count
  

•    * Returns: 0 on success or negative value of errno.
  

•    */
  

•   int (*get_cnt)(struct udevice *dev, int type, int *cnt);
  

get_count

Return count in return value instead of a parameter.

...

•   /**
  

•    * Regulator output value descriptor is specific for each output.
  

•    * It's usually driver static data, but output value limits are
  

•    * often defined in the device-tree, so those are platform dependent
  

•    * attributes.
  

•    *
  

•    * get_value_desc - get value descriptor of the given regulator output
  

•    * @dev           - regulator device
  

•    * @type          - regulator type, one of enum regulator_type
  

•    * @number        - regulator number
  

•    * Gets:
  

•    * @desc          - pointer to the descriptor
  

•    * Returns: 0 on success or negative value of errno.
  

•    */
  

•   int (*get_value_desc)(struct udevice *dev, int type, int number,
  

•                         struct regulator_value_desc **desc);
  

You could use descp, to indicate it returns a pointer.

...

•   /**
  

•    * Regulator output mode descriptors are device specific.
  

•    * It's usually driver static data.
  

•    * Each regulator output can support different mode types,
  

•    * so usually will return an array with a number 'mode_desc_cnt'
  

•    * of mode descriptors.
  

•    *
  

•    * get_mode_desc_array - get mode descriptor array of the given
  

•    *                       regulator output number
  

•    * @dev                - regulator device
  

•    * @type               - regulator type, one of 'enum regulator_type'
  

•    * @number             - regulator number
  

regulator_num would be better

...

•    * Gets:
  

•    * @mode_desc_cnt      - descriptor array entry count
  

•    * @desc               - pointer to the descriptor array
  

•    * Returns: 0 on success or negative value of errno.
  

•    */
  

•   int (*get_mode_desc_array)(struct udevice *dev, int type,
  

So should type be an enum?

...

•                              int number, int *mode_desc_cnt,
  

•                              struct regulator_mode_desc **desc);
  

•   /**
  

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

•    * however the regulator commands operates on a mili Volt unit.
  

•    *
  

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

•    * @dev          - regulator device
  

•    * @type         - regulator type, one of 'enum regulator_type'
  

•    * @number       - regulator number
  

•    * Sets/Gets:
  

•    * @value        - set or get output value
  

•    * Returns: 0 on success or negative value of errno.
  

•    */
  

•   int (*get_value)(struct udevice *dev, int type, int number, int *value);
  

Return value in return value instead of a parameter.

...

•   int (*set_value)(struct udevice *dev, int type, int number, int value);
  

•   /**
  

•    * The most basic feature of the regulator output is it's on/off state.
  

its

...

•    *
  

•    * get/set_state - get/set on/off state of the given output number
  

•    * @dev          - regulator device
  

•    * @type         - regulator type, one of 'enum regulator_type'
  

•    * @number       - regulator number
  

•    * Sets/Gets:
  

•    * @state        - set or get one of 'enum regulator_out_state'
  

•    * Returns: 0 on success or negative value of errno.
  

•    */
  

•   int (*get_state)(struct udevice *dev, int type, int number, int *state);
  

Return state in return value instead of a parameter.

...

•   int (*set_state)(struct udevice *dev, int type, int number, int state);
  

•   /**
  

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

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

•    * field 'mode' of struct regulator_mode_desc.
  

•    *
  

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

•    * @dev          - regulator device
  

•    * @type         - regulator type, one of 'enum regulator_type'
  

•    * @number       - regulator number
  

•    * Sets/Gets:
  

•    * @mode         - set or get output mode
  

•    * Returns: 0 on success or negative value of errno.
  

•    */
  

•   int (*get_mode)(struct udevice *dev, int type, int number, int *mode);
  

Return mode in return value.

...

•   int (*set_mode)(struct udevice *dev, int type, int number, int mode);
  

+};

+/**

• • regulator_get_cnt: returns the number of driver registered regulators.

driver-registered

...

• • This is used for pmic regulator command purposes.
• • @dev - pointer to the regulator device
• • @type - regulator type: device specific
• • Gets:
• • @cnt - regulator count
• • Returns: 0 on success or negative value of errno.
• */

+int regulator_get_cnt(struct udevice *dev, int type, int *cnt);

regulator_get_count. Return count in return value instead of a parameter.

...

+/**

• • regulator_get_value_desc: returns a pointer to the regulator value
• • descriptor of a given device regulator output number.
• • @dev - pointer to the regulator device
• • @type - regulator descriptor type
• • @number - descriptor number (regulator output number)
• • Gets:
• • @desc - pointer to the descriptor
• • Returns: 0 on success or negative value of errno.
• */

+int regulator_get_value_desc(struct udevice *dev, int type, int number,

•                        struct regulator_value_desc **desc);
  

+/**

• • regulator_get_mode_desc: returns a pointer to the array of regulator mode
• • descriptors of a given device regulator type and number.
• • @dev - pointer to the regulator device
• • @type - regulator type: device specific
• • @number - output number: device specific
• • Gets:
• • @mode_cnt - descriptor array entry count
• • @desc - pointer to the descriptor array
• • Returns: 0 on success or negative value of errno.
• */

+int regulator_get_mode_desc(struct udevice *dev, int type, int number,

•                       int *mode_cnt, struct regulator_mode_desc **desc);
  

+/**

• • regulator_get_value: returns voltage value of a given device
• • regulator type number.
• • @dev - pointer to the regulator device
• • @type - regulator type: device specific
• • @number - output number: device specific
• • Gets:
• • @val - returned voltage - unit: uV (micro Volts)
• • Returns: 0 on success or negative value of errno.

You may as well return the voltage in the return value, unless it can be -ve (I assume not?)

...

• */

+int regulator_get_value(struct udevice *dev, int type, int number, int *val);

+/**

• • regulator_set_value: set the voltage value of a given device regulator type
• • number to the given one passed by the argument: @val
• • @dev - pointer to the regulator device
• • @type - regulator type: device specific
• • @number - output number: device specific
• • @val - voltage value to set - unit: uV (micro Volts)
• • Returns - 0 on success or -errno val if fails
• */

+int regulator_set_value(struct udevice *dev, int type, int number, int val);

+/**

• • regulator_get_state: returns output state of a given device
• • regulator type number.
• • @dev - pointer to the regulator device
• • @type - regulator type, device specific
• • @number - regulator number, device specific
• • Gets:
• • @state - returned regulator number output state:

• •           - REGULATOR_OFF (0) - disable
    

• •           - REGULATOR_ON  (1) - enable
    

• • Returns: - 0 on success or -errno val if fails
• */

+int regulator_get_state(struct udevice *dev, int type, int number, int *state);

Return state in return value instead of a parameter.

...

+/**

• • regulator_set_state: set output state of a given device regulator type
• • number to the given one passed by the argument: @state
• • @dev - pointer to the regulator device
• • @type - regulator type: device specific
• • @number - output number: device specific
• • @state - output state to set

• •               - enum REGULATOR_OFF == 0 - disable
    

• •               - enum REGULATOR_ON  == 1 - enable
    

• • Returns - 0 on success or -errno val if fails
• */

+int regulator_set_state(struct udevice *dev, int type, int number, int state);

+/**

• • regulator_get_mode: get mode number of a given device regulator type number
• • @dev - pointer to the regulator device
• • @type - output number: device specific
• • @number - output number: device specific
• • Gets:
• • @mode - returned mode number
• • Returns - 0 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 'regulator_mode_desc' has
• • a mode field for this purpose and register_value for a raw register value.
• */

+int regulator_get_mode(struct udevice *dev, int type, int number, int *mode);

Return mode in return value instead of a parameter.

...

+/**

• • regulator_set_mode: set given regulator type and number mode to the given
• • one passed by the argument: @mode
• • @dev - pointer to the regulator device
• • @type - output number: device specific
• • @number - output number: device specific
• • @mode - mode type (struct regulator_mode_desc.mode)
• • 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 type, int number, int mode);

+/**

• • regulator_..._get: returns the pointer to the pmic regulator device
• • by specified arguments:
• • NAME:
• • @name - device name (useful for specific names)
• • or SPI/I2C interface:
• • @bus - device I2C/SPI bus number
• • @addr_cs - device specific address for I2C or chip select for SPI,

• •         on the given bus number
    

• • Gets:
• • @regulator - pointer to the pmic device
• • Returns: 0 on success or negative value of errno.
• • The returned 'regulator' device can be used with:
• • • regulator_get/set_*
• • and if pmic_platdata is given, then also with:
• • • pmic_if_* calls
• • The last one is used for the pmic command purposes.
• */

+int regulator_get(char *name, struct udevice **regulator); +int regulator_i2c_get(int bus, int addr, struct udevice **regulator); +int regulator_spi_get(int bus, int cs, struct udevice **regulator);

Hoping you don't need these last two!

...

+#endif /* _INCLUDE_REGULATOR_H_ */

1.9.1

Regards, Simon

Thank you,