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

Hello Simon,

On 03/29/2015 03:07 PM, Simon Glass wrote:

Hi Prazemyslaw,

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

...

This is an introduction to driver-model multi uclass PMIC support. It starts with UCLASS_PMIC - a common PMIC devices uclass type to provide device read/write operations only.

Your Kconfig docs are a model to others! It describes the function very nicely without a lot of words.

Please excuse the nits, they are intended to help it read better.

That's ok, it's better to fix if needs.

...

Beside two basic operations the pmic platform data is introduced, which provides basic informations about the pmic device I/O interface and is shared with all childs (and should also for childs new uclass types in the future).

Usually PMIC devices provides various functionalities with single or multiple I/O interfaces. Using this new framework and new uclass types introduced in the future, it can be handle like this:

_ root device | |_ BUS 0 device (e.g. I2C0) - UCLASS_I2C/SPI/... | |_ PMIC device 1 (read/write ops) - UCLASS_PMIC | |_ REGULATOR device (ldo/buck/... ops) - UCLASS_REGULATOR | |_ CHARGER device (charger ops) - UCLASS_CHARGER (in the future) | |_ MUIC device (microUSB con ops) - UCLASS_MUIC (in the future) | |_ ... | |_ BUS 1 device (e.g. I2C1) - UCLASS_I2C/SPI/... |_ PMIC device 2 (read/write ops) - UCLASS_PMIC |_ RTC device (rtc ops) - UCLASS_MUIC (in the future)

For each PMIC device interface, new UCLASS_PMIC device is bind with proper pmic driver, and it's child devices provides some specified operations.

All new definitions can be found in file:

• 'include/power/pmic.h'

Uclass file:

• pmic-uclass.c - provides a common code for UCLASS_PMIC device drivers

The old pmic framework is still kept and is independent.

Changes:

• new uclass-id: UCLASS_PMIC
• new config: CONFIG_DM_PMIC

New pmic api is documented in: doc/driver-model/pmic-framework.txt

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

Changes V2:

• pmic uclass: adjust uclass code to the mainline changes
• pmic uclass: remove pmic_i2c and pmic_spi
• pmic uclass: modify pmic_platdata
• pmic uclass: add pmic_if_* functions
• pmic uclass: remove pmic_init_dm()
• pmic uclass: cleanup
• pmic.h: define pmic ops structure (read/write operations)
• pmic.h: add comments to functions

Changes V3:

• pmic-uclass.c and pmic.h: -- remove pmic_if_* functions -- add new function pmic_child_node_scan()
• add Kconfig entry

---

drivers/power/Kconfig | 68 ++++++++++++++ drivers/power/Makefile | 1 + drivers/power/pmic-uclass.c | 130 +++++++++++++++++++++++++++ include/dm/uclass-id.h | 3 + include/power/pmic.h | 210 ++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 412 insertions(+) create mode 100644 drivers/power/pmic-uclass.c

diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig index f8f0239..3513b46 100644 --- a/drivers/power/Kconfig +++ b/drivers/power/Kconfig @@ -1,3 +1,71 @@ +config DM_PMIC

•   bool "Enable Driver Model for PMIC drivers (UCLASS_PMIC)"
  

•   depends on DM
  

•   ---help---
  

•   This config enables the driver-model multi uclass PMIC support.
  

•   Its basic uclass type is: UCLASS_PMIC, which is designed to provide
  

•   a common I/O interface for pmic child devices of various uclass types.
  

Should be consistent - and use PMIC instead pmic.

...

•   Usually PMIC IC's provides more than one functionality, which means
  

s/functionality/function/

...

•   that we should implement new uclass operations for each one. Usually
  

•   PMIC's provide those various functionalities by one or more interfaces.
  

functions

...

•   And this could looks like this:
  

•   root device
  

•   |_ BUS 0 device (e.g. I2C0)                 - UCLASS_I2C/SPI/...
  

•   | |_ PMIC device (READ/WRITE ops)           - UCLASS_PMIC
  

•   |   |  (pmic sub-devices)
  

•   |   |_ REGULATOR device (ldo/buck/... ops)  - UCLASS_REGULATOR
  

•   |   |_ CHARGER device (charger ops)         - UCLASS_CHARGER (future)
  

•   |   |_ MUIC device (microUSB connector ops) - UCLASS_MUIC    (future)
  

•   |   |_ ...
  

•   |
  

•   |_ BUS 1 device (e.g. I2C1)                 - UCLASS_I2C/SPI/...
  

•      |_ PMIC device (READ/WRITE ops)          - UCLASS_PMIC
  

•        |  (pmic sub-devices)
  

•        |_ RTC device (rtc ops)                - UCLASS_MUIC (future)
  

Would this be UCLASS_RTC?

...

•   From the I/O interface point of view, there can be found two PMIC types:
  

•   - single I/O interface - then UCLASS_PMIC device should be a parent of
  

•     all pmic sub-devices, where each is usually different uclass type, but
  

•     need to access the same interface
  

•   - multiple I/O interfaces - for each interface the UCLASS_PMIC device
  

•     should be a parent of only those devices (different uclass types),
  

•     which needs to access the specified interface.
  

•   For each case, binding should be done automatically. If only device tree
  

Remove the word 'only'?

...

•   nodes/subnodes are proper defined, then:
  

how about a blank line here?

...

•   |_ the ROOT driver will bind the device for I2C/SPI node:
  

•     |_ the I2C/SPI driver should bind a device for pmic node:
  

•       |_ the PMIC driver should bind devices for its childs:
  

•         |  (pmic sub-devices)
  

•         |_ regulator (child)
  

•         |_ charger   (child)
  

•         |_ other     (child)
  

•   The same for other I/O bus nodes, if pmic uses more then one interface.
  

•   Note:
  

•   Each PMIC interface driver should use different compatible string.
  

use a different

...

•   There are few basic functions in the UCLASS_PMIC driver API, declared
  

There are a few

...

•   in the file 'include/power/pmic.h':
  

•    - int pmic_get(...);
  

•    - int pmic_read(struct udevice *pmic, ...);
  

•    - int pmic_write(struct udevice *pmic, ...);
  

•   For the simple implementation, in some cases the pmic uclass device,
  

•   can be self-sufficient to drive the PMIC functionality. In other case,
  

s/self-sufficient/enough/

...

•   if each pmic sub-device(child) driver need access to the pmic specified
  

PMIC-specified

...

•   registers, it need to know only the register address and then the access
  

s/to//

...

•   is done through the parent pmic driver. Like in the example:
  

s/is/can be/

For example: (blank line here?)

Thanks, will fix the all above.

...

•   _ root driver
  

•   |_ dev: bus I2C0                                   - UCLASS_I2C
  

•   | |_ dev: my_pmic        (read/write)     (parent) - UCLASS_PMIC
  

•   |   |_ dev: my_regulator (set value/etc.) (child)  - UCLASS_REGULATOR
  

•   So the call will looks like below:
  

•   'pmic_write(regulator->parent, addr, value, len);'
  

• config AXP221_POWER boolean "axp221 / axp223 pmic support" depends on MACH_SUN6I || MACH_SUN8I

diff --git a/drivers/power/Makefile b/drivers/power/Makefile index 2145652..5c9a189 100644 --- a/drivers/power/Makefile +++ b/drivers/power/Makefile @@ -21,3 +21,4 @@ obj-$(CONFIG_DIALOG_POWER) += power_dialog.o 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 diff --git a/drivers/power/pmic-uclass.c b/drivers/power/pmic-uclass.c new file mode 100644 index 0000000..df49a4b --- /dev/null +++ b/drivers/power/pmic-uclass.c @@ -0,0 +1,130 @@ +/*

• • 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 <dm/device-internal.h> +#include <dm/uclass-internal.h> +#include <dm/root.h> +#include <dm/lists.h> +#include <compiler.h> +#include <errno.h>

+DECLARE_GLOBAL_DATA_PTR;

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

•   struct udevice *dev;
  

•   int ret;
  

•   *devp = NULL;
  

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

•        dev;
  

•        ret = uclass_next_device(&dev)) {
  

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

•                   *devp = dev;
  

•                   return ret;
  

•           }
  

•   }
  

•   return -ENODEV;
  

This function will probe every PMIC, looking for the correct one.

I think you should change pmic_get() to call a core function in uclass.c, like uclass_get_device_by_name(). It should use uclass_foreach_dev() so that it doesn't probe anything except the one it finds.You can use uclass_get_device_tail() at the end of that function - see uclass_get_device_by_of_offset() as an example.

Right, this is not good. The sequence, which you described, was in V2 of this code. I will adjust it and move to uclass.c.

...

+}

+int pmic_reg_count(struct udevice *dev) +{

•   const struct dm_pmic_ops *ops;
  

•   ops = pmic_get_uclass_ops(dev, UCLASS_PMIC);
  

•   if (!ops)
  

•           return -ENODEV;
  

•   return ops->reg_count;
  

+}

+int pmic_read(struct udevice *dev, uint reg, uint8_t *buffer, int len) +{

•   const struct dm_pmic_ops *ops;
  

•   if (!buffer)
  

•           return -EFAULT;
  

•   ops = pmic_get_uclass_ops(dev, UCLASS_PMIC);
  

•   if (!ops)
  

•           return -ENODEV;
  

-ENOSYS, although IMO assert() would be OK.

...

•   if (!ops->read)
  

•           return -EPERM;
  

-ENOSYS

...

•   if (ops->read(dev, reg, buffer, len))
  

•           return -EIO;
  

We must not swallow errors.

ret = ops->read(...) if (ret) return ret;

Please adjust below also.

Right, will fix.

...

•   return 0;
  

+}

+int pmic_write(struct udevice *dev, uint reg, uint8_t *buffer, int len) +{

•   const struct dm_pmic_ops *ops;
  

•   if (!buffer)
  

•           return -EFAULT;
  

•   ops = pmic_get_uclass_ops(dev, UCLASS_PMIC);
  

•   if (!ops)
  

•           return -ENODEV;
  

•   if (!ops->write)
  

•           return -EPERM;
  

•   if (ops->write(dev, reg, buffer, len))
  

•           return -EIO;
  

•   return 0;
  

+}

+int pmic_child_node_scan(struct udevice *parent,

•                    const char *optional_subnode,
  

•                    const char *compatible_property_name)
  

+{

•   const void *blob = gd->fdt_blob;
  

•   struct udevice *childp;
  

•   int offset = parent->of_offset;
  

•   int childs = 0;
  

•   int ret;
  

•   debug("%s: bind child for %s\n", __func__, parent->name);
  

•   if (optional_subnode) {
  

•           debug("Looking for %s optional subnode: %s \n", parent->name,
  

•                 optional_subnode);
  

•           offset = fdt_subnode_offset(blob, offset, optional_subnode);
  

•           if (offset <= 0) {
  

•                   debug("Pmic: %s subnode: %s not found!",
  

•                         parent->name, optional_subnode);
  

•                   return -ENXIO;
  

•           }
  

•   }
  

•   for (offset = fdt_first_subnode(blob, offset); offset > 0;
  

•        offset = fdt_next_subnode(blob, offset)) {
  

•           ret = lists_bind_fdt_by_prop(parent, blob, offset,
  

•                                        compatible_property_name, &childp);
  

•           if (ret)
  

•                   continue;
  

•           childs++;
  

•   }
  

•   return childs;
  

+}

+UCLASS_DRIVER(pmic) = {

•   .id             = UCLASS_PMIC,
  

•   .name           = "pmic",
  

+}; diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index 91bb90d..3ecfa23 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -35,6 +35,9 @@ enum uclass_id { UCLASS_I2C_EEPROM, /* I2C EEPROM device */ UCLASS_MOD_EXP, /* RSA Mod Exp device */

•   /* PMIC uclass and PMIC-related uclass types */
  

This comment seemed confusing - is it for PMICs or other things also?

Ah, my mistake when rebasing the code. This should be added with regulator uclass.

...

•   UCLASS_PMIC,
  

•    UCLASS_COUNT,
     UCLASS_INVALID = -1,
  

  };

diff --git a/include/power/pmic.h b/include/power/pmic.h index afbc5aa..b55304f 100644 --- a/include/power/pmic.h +++ b/include/power/pmic.h @@ -1,4 +1,7 @@ /*

• • Copyright (C) 2014-2015 Samsung Electronics
• • Przemyslaw Marczak p.marczak@samsung.com
• • Copyright (C) 2011-2012 Samsung Electronics
  • Lukasz Majewski l.majewski@samsung.com

@@ -9,10 +12,13 @@ #define __CORE_PMIC_H_

#include <linux/list.h> +#include <spi.h> #include <i2c.h> #include <power/power_chrg.h>

enum { PMIC_I2C, PMIC_SPI, PMIC_NONE};

+#ifdef CONFIG_POWER enum { I2C_PMIC, I2C_NUM, }; enum { PMIC_READ, PMIC_WRITE, }; enum { PMIC_SENSOR_BYTE_ORDER_LITTLE, PMIC_SENSOR_BYTE_ORDER_BIG, }; @@ -77,7 +83,210 @@ struct pmic { struct pmic *parent; struct list_head list; }; +#endif /* CONFIG_POWER */

+#ifdef CONFIG_DM_PMIC +/**

• • Driver model pmic framework.
• • The PMIC_UCLASS uclass is designed to provide a common I/O
• • interface for pmic child devices of various uclass types.

I worry about having the docs in multiple places. Should you adjust this to point to the Kconfig? Or change the Kconfig to point here? Or maybe it would be better to drop both and put these in your README? I am concerned that if we later change something, we end up with inconsistent docs.

Right, this could be an issue in the future. I suppose that first thing is to look at this header file, before anyone starts work with this framework. So maybe it's better keep the description only in this file, and add some basic info to Kconfig and doc, with the links to here?

...

• • Usually PMIC devices provides more than one functionality,
• • which means that we should implement uclass operations for
• • each functionality - one driver per uclass.
• • And usually PMIC devices provide those various functionalities
• • by one or more interfaces. And this could looks like this:
• *_ root device
• • |_ BUS 0 device (e.g. I2C0) - UCLASS_I2C/SPI/...
• • | |_ PMIC device (READ/WRITE ops) - UCLASS_PMIC
• • | |_ REGULATOR device (ldo/buck/... ops) - UCLASS_REGULATOR
• • | |_ CHARGER device (charger ops) - UCLASS_CHARGER (in the future)
• • | |_ MUIC device (microUSB connector ops) - UCLASS_MUIC (in the future)
• • | |_ ...
• • |
• • |_ BUS 1 device (e.g. I2C1) - UCLASS_I2C/SPI/...
• • |_ PMIC device (READ/WRITE ops) - UCLASS_PMIC

• • |_ RTC device (rtc ops)                 - UCLASS_MUIC (in the future)
    

• • Two PMIC types are possible:
• • • single I/O interface
• • Then UCLASS_PMIC device should be a parent of all pmic devices, where each
• • is usually different uclass type, but need to access the same interface
• • • multiple I/O interfaces
• • For each interface the UCLASS_PMIC device should be a parent of only those
• • devices (different uclass types), which needs to access the specified
• • interface.
• • For each case binding should be done automatically. If only device tree
• • nodes/subnodes are proper defined, then:
• • |_ the ROOT driver will bind the device for I2C/SPI node:
• • |_ the I2C/SPI driver should bind a device for pmic node:

• • |_ the PMIC driver should bind devices for its childs:
    

• •   |_ regulator (child)
    

• •   |_ charger   (child)
    

• •   |_ other     (child)
    

• • The same for other bus nodes, if pmic uses more then one interface.
• • Note:
• • Each PMIC interface driver should use different compatible string.
• • If each pmic child device driver need access the pmic specified registers,
• • it need to know only the register address and the access is done through
• • the parent pmic driver. Like in the example:
• *_ root driver
• • |_ dev: bus I2C0 - UCLASS_I2C
• • | |_ dev: my_pmic (read/write) (is parent) - UCLASS_PMIC
• • | |_ dev: my_regulator (set value/etc..) (is child) - UCLASS_REGULATOR
• • To ensure such device relationship, the pmic device driver should also bind
• • all its child devices, like in the example below. It should be by the call
• • to 'pmic_child_node_scan()' - it allows bind in optional subnode and with
• • optional compatible property, e.g. "regulator-compatible" or "regulator".
• • my_pmic.c - pmic driver:
• • int my_pmic_bind(struct udevice *my_pmic)
• • {
• • ...

• •  ret = pmic_child_node_scan(my_pmic, NULL, "compatible");
    

• •             ...
    

• • ...
• • }
• • my_regulator.c - regulator driver (child of pmic I/O driver):
• • int my_regulator_set_value(struct udevice *dev, int type, int num, int *val)
• • {
• • ...

• •     some_val = ...;
    

• •     // dev->parent == my_pmic
    

• •     pmic_write(dev->parent, some_reg, some_val);
    

• • ...
• • }
• • In this example pmic driver is always a parent for other pmic devices.
• */

+/**

• • struct dm_pmic_ops - PMIC device I/O interface
• • Should be implemented by UCLASS_PMIC device drivers. The standard
• • device operations provides the I/O interface for it's childs.
• • @reg_count: devices register count
• • @read: read 'len' bytes at "reg" and store it into the 'buffer'
• • @write: write 'len' bytes from the 'buffer' to the register at 'reg' address
• */

+struct dm_pmic_ops {

•   int reg_count;
  

•   int (*read)(struct udevice *dev, uint reg, uint8_t *buffer, int len);
  

•   int (*write)(struct udevice *dev, uint reg, uint8_t *buffer, int len);
  

nit: const uint8_t *buffer?

Right, will fix this.

...

+};

+/* enum pmic_op_type - used for various pmic devices operation calls,

• • for reduce a number of lines with the same code for read/write or get/set.
• • @PMIC_OP_GET - get operation
• • @PMIC_OP_SET - set operation

+*/ +enum pmic_op_type {

•   PMIC_OP_GET,
  

•   PMIC_OP_SET,
  

+};

+/**

• • pmic_get_uclass_ops - inline function for getting device uclass operations
• • @dev - device to check
• • @uclass_id - device uclass id
• • Returns:
• • void pointer to device operations or NULL pointer on error
• */

+static __inline__ const void *pmic_get_uclass_ops(struct udevice *dev,

•                                             int uclass_id)
  

+{

•   const void *ops;
  

•   if (!dev)
  

•           return NULL;
  

•   if (dev->driver->id != uclass_id)
  

•           return NULL;
  

•   ops = dev->driver->ops;
  

•   if (!ops)
  

•           return NULL;
  

•   return ops;
  

+}

+/* drivers/power/pmic-uclass.c */

+/**

• • pmic_child_node_scan - scan the pmic node or its 'optional_subnode' for its
• • childs, by the compatible property name given by arg 'compatible_prop_name'.
• • Few dts files with a different pmic regulators, uses different property
• • name for its driver 'compatible' string, like:
• • • 'compatible'
• • 'regulator-compatible'
• • The pmic driver should should bind its devices by proper compatible property
• • name.
• • @parent - pmic parent device (usually UCLASS_PMIC)
• • @optional_subnode - optional pmic subnode with the childs within it
• • @compatible_property_name - e.g.: 'compatible'; 'regulator-compatible', ...

@return

Ok

...

• */

+int pmic_child_node_scan(struct udevice *parent,

•                    const char *optional_subnode,
  

•                    const char *compatible_property_name);
  

+/**

• • pmic_get: get the pmic device using its name
• • @name - device name
• • @devp - returned pointer to the pmic device
• • Returns: 0 on success or negative value of errno.

• @return 0 on success or negative value of errno.

...

• • The returned devp device can be used with pmic_read/write calls
• */

+int pmic_get(char *name, struct udevice **devp);

+/**

• • pmic_reg_count: get the pmic register count
• • The required pmic device can be obtained by 'pmic_get()'
• • @dev - pointer to the UCLASS_PMIC device
• • Returns: register count value on success or negative value of errno.
• */

+int pmic_reg_count(struct udevice *dev);

+/**

• • pmic_read/write: read/write to the UCLASS_PMIC device
• • The required pmic device can be obtained by 'pmic_get()'
• • @pmic - pointer to the UCLASS_PMIC device
• • @reg - device register offset
• • @val - value for write or its pointer to read

This doesn't match the functions.

Ah, missed this - will fix

...

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

+int pmic_read(struct udevice *dev, uint reg, uint8_t *buffer, int len); +int pmic_write(struct udevice *dev, uint reg, uint8_t *buffer, int len);

const uint8_t buffer for pmic_write()?

Yes, will adjust to ops prototypes.

...

+#endif /* CONFIG_DM_PMIC */

+#ifdef CONFIG_POWER int pmic_init(unsigned char bus); int power_init_board(void); int pmic_dialog_init(unsigned char bus); @@ -88,6 +297,7 @@ int pmic_probe(struct pmic *p); int pmic_reg_read(struct pmic *p, u32 reg, u32 *val); int pmic_reg_write(struct pmic *p, u32 reg, u32 val); int pmic_set_output(struct pmic *p, u32 reg, int ldo, int on); +#endif

#define pmic_i2c_addr (p->hw.i2c.addr)

#define pmic_i2c_tx_num (p->hw.i2c.tx_num)

1.9.1

Regards, Simon

Thanks,