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

[Simon Glass <sjg@chromium.org>]

Hi Przemyslaw,

On 3 March 2015 at 09:24, 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.
>
> 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/README.power-framework-dm
>
> 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
> ---
>  drivers/power/Makefile      |   1 +
>  drivers/power/pmic-uclass.c | 191 +++++++++++++++++++++++++++++++
>  include/dm/uclass-id.h      |   3 +
>  include/power/pmic.h        | 265 ++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 460 insertions(+)
>  create mode 100644 drivers/power/pmic-uclass.c

This should have a Kconfig file.

>
> 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..309463e
> --- /dev/null
> +++ b/drivers/power/pmic-uclass.c
> @@ -0,0 +1,191 @@
> +/*
> + * 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;
> +
> +static char * const pmic_interfaces[] = {
> +       "I2C",
> +       "SPI",
> +       "--",
> +};
> +
> +const char *pmic_if_str(struct udevice *pmic)
> +{
> +       int if_types = ARRAY_SIZE(pmic_interfaces);
> +       int if_type;
> +
> +       if_type = pmic_if_type(pmic);
> +       if (if_type < 0 || if_type >= if_types)
> +               return pmic_interfaces[if_types - 1];
> +
> +       return pmic_interfaces[if_type];
> +}
> +
> +int pmic_if_type(struct udevice *pmic)
> +{
> +       struct pmic_platdata *pl = pmic->platdata;
> +
> +       return pl->if_type;
> +}
> +
> +int pmic_if_bus_num(struct udevice *pmic)
> +{
> +       struct pmic_platdata *pl = pmic->platdata;
> +
> +       return pl->if_bus_num;
> +}
> +
> +int pmic_if_addr_cs(struct udevice *pmic)
> +{
> +       struct pmic_platdata *pl = pmic->platdata;
> +
> +       return pl->if_addr_cs;
> +}
> +
> +int pmic_if_max_offset(struct udevice *pmic)
> +{
> +       struct pmic_platdata *pl = pmic->platdata;
> +
> +       return pl->if_max_offset;
> +}
> +
> +int pmic_read(struct udevice *pmic, unsigned reg, unsigned char *val)
> +{
> +       const struct dm_pmic_ops *ops;
> +
> +       ops = pmic_get_uclass_ops(pmic, UCLASS_PMIC);
> +       if (!ops)
> +               return -ENODEV;
> +
> +       if (!ops->read)
> +               return -EPERM;
> +
> +       if (ops->read(pmic, reg, val))
> +               return -EIO;
> +
> +       return 0;
> +}
> +
> +int pmic_write(struct udevice *pmic, unsigned reg, unsigned char val)
> +{
> +       const struct dm_pmic_ops *ops;
> +
> +       ops = pmic_get_uclass_ops(pmic, UCLASS_PMIC);
> +       if (!ops)
> +               return -ENODEV;
> +
> +       if (!ops->write)
> +               return -EPERM;
> +
> +       if (ops->write(pmic, reg, val))
> +               return -EIO;
> +
> +       return 0;
> +}
> +
> +int pmic_get(char *name, struct udevice **pmic)
> +{
> +       struct udevice *dev;
> +       struct uclass *uc;
> +       int ret;
> +
> +       ret = uclass_get(UCLASS_PMIC, &uc);
> +       if (ret) {
> +               error("PMIC 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);
> +                       *pmic = dev;
> +                       return ret;
> +               }
> +       }
> +
> +       *pmic = NULL;
> +       return -EINVAL;
> +}
> +
> +int pmic_i2c_get(int bus, int addr, struct udevice **pmic)
> +{
> +       struct udevice *dev;
> +       int ret;
> +
> +       ret = i2c_get_chip_for_busnum(bus, addr, 1, &dev);
> +       if (ret) {
> +               error("Chip: %d on bus %d not found!", addr, bus);
> +               *pmic = NULL;
> +               return ret;
> +       }
> +
> +       *pmic = dev;
> +       return 0;
> +}
> +
> +int pmic_spi_get(int bus, int cs, struct udevice **pmic)
> +{
> +       struct udevice *busp, *devp;
> +       int ret;
> +
> +       ret = spi_find_bus_and_cs(bus, cs, &busp, &devp);
> +       if (ret) {
> +               error("Chip: %d on bus %d not found!", cs, bus);
> +               *pmic = NULL;
> +               return ret;
> +       }
> +
> +       *pmic = devp;
> +       return 0;
> +}
> +
> +int pmic_io_dev(struct udevice *pmic, struct udevice **pmic_io)
> +{
> +       struct pmic_platdata *pl;
> +       int ret;
> +
> +       pl = dev_get_platdata(pmic);
> +       if (!pl) {
> +               error("pmic: %s platdata not found!", pmic->name);
> +               return -EINVAL;
> +       }
> +
> +       switch (pl->if_type) {
> +       case PMIC_I2C:
> +               ret = pmic_i2c_get(pl->if_bus_num, pl->if_addr_cs, pmic_io);
> +               break;
> +       case PMIC_SPI:
> +               ret = pmic_spi_get(pl->if_bus_num, pl->if_addr_cs, pmic_io);
> +               break;
> +       default:
> +               error("Unsupported interface for: %s", pmic->name);
> +               ret = -ENXIO;
> +       }
> +
> +       return ret;
> +}
> +
> +UCLASS_DRIVER(pmic) = {
> +       .id             = UCLASS_PMIC,
> +       .name           = "pmic",
> +};
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index 91bb90d..ff360ac 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 uclasses */

PMIC-related

> +       UCLASS_PMIC,
> +
>         UCLASS_COUNT,
>         UCLASS_INVALID = -1,
>  };
> diff --git a/include/power/pmic.h b/include/power/pmic.h
> index afbc5aa..94171ac 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,265 @@ 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.
> + *
> + * 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 this example:
> + *
> + * my_pmic.c - pmic driver:
> + *
> + * int my_pmic_bind(struct udevice *my_pmic)
> + * {
> + *         // A list of other drivers for this pmic
> + *         char *my_pmic_drv_list = {"my_regulator", "my_charger", "my_rtc"};
> + *         struct udevice *my_pmic_child_dev[3];
> + * ...
> + *         for (i=0; i < ARRAY_SIZE(my_pmic_drv_list); i++) {
> + *                 // Look for child device driver
> + *                 drv_my_reg = lists_driver_lookup_name(my_pmic_drv_list[i]);
> + *
> + *                 // If driver found, then bind the new device to it
> + *                 if (drv_my_reg)
> + *                         ret = device_bind(my_pmic, my_pmic_drv_list[i],
> + *                                           my_pmic->name, my_pmic->platdata,
> + *                                           my_pmic->of_offset,
> + *                                           my_pmic_child_dev[i]);
> + *                 ...
> + *         }

Can we not automatically do this with dm_scan_fdt_node() as in
spi_post_bind(), etc.?

> + * ...
> + * }
> + *
> + * 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,
> + * because the first argument of device_bind() call is the parent device,
> + * and here it is 'my_pmic'.
> + *
> + * For all childs the device->platdata is the same, and provide an info about
> + * the same interface - it's used for getting the devices by interface address
> + * and uclass types.
> + */
> +
> +/**
> + * struct pmic_platdata - PMIC chip interface info
> + *
> + * This is required for pmic command purposes and should be shared
> + * by pmic device and it's childs as the same platdata pointer.
> + *
> + * @if_type:       one of: PMIC_I2C, PMIC_SPI, PMIC_NONE
> + * @if_bus_num:    usually alias seq of i2c/spi bus device
> + * @if_addr_cd:    i2c/spi chip addr or cs
> + * @if_max_offset: max register offset within the chip
> + */
> +struct pmic_platdata {
> +       int if_type;
> +       int if_bus_num;
> +       int if_addr_cs;
> +       int if_max_offset;
> +};

Can we drop the if_ prefix and use an enum for if_type?

For bus_num, this will be the parent of the pmic. There should be no
need to store it.

For addr_cs, there should be no need to store this. The pmic will be
either on an i2c or spi bus - both of these provide the address
information in dev_get_parent_platdata().

if_max_offset should be in the PMIC platdata but not that of its
children. So I don't think they should all use the same platdata.

> +
> +/**
> + * 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.
> + *
> + * @read:   called for read "reg" value into "val" through "pmic" parent
> + * @write:  called for write "val" into "reg" through the "pmic" parent
> + */
> +struct dm_pmic_ops {
> +       int (*read)(struct udevice *pmic, unsigned reg, unsigned char *val);

Can any pmics support reading 16 bits or more?

> +       int (*write)(struct udevice *pmic, unsigned reg, unsigned char val);
> +};
> +
> +/* enum pmic_op_type - used for various pmic devices operation calls,

Comment style

> + * 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)

enum

> +{
> +       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_..._get: looking for the pmic device using the specified arguments:
> + *
> + * NAME:
> + *  @name    - device name (useful for single 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
> + *
> + * Returns:
> + * @pmic    - returned pointer to the pmic device
> + * Returns: 0 on success or negative value of errno.
> + *
> + * The returned pmic device can be used with:
> + * - pmic_read/write calls
> + * - pmic_if_* calls
> + */
> +int pmic_get(char *name, struct udevice **pmic);
> +int pmic_i2c_get(int bus, int addr, struct udevice **pmic);
> +int pmic_spi_get(int bus, int cs, struct udevice **pmic);

These last two should be internal to the pmic framework I think.
Clients should not care.

Also we should reference SPI and I2C devices with a struct udevice,
not a bus/addr or bus/cs pair.

> +
> +/**
> + * pmic_io_dev: returns given pmic/regulator, uclass pmic I/O udevice
> + *
> + * @pmic    - pointer to pmic_io child device
> + *
> + * Returns:
> + * @pmic_io - pointer to the I/O chip device
> + *
> + * This is used for pmic command, and also can be used if
> + * needs raw read/write operations for uclass regulator device.
> + * Example of use:
> + *  pmic_io_dev(regulator, &pmic_io);
> + *  pmic_write(pmic_io, 0x0, 0x1);
> + *
> + * This base on pmic_platdata info, so for the given pmic i2c/spi chip,
> + * as '@pmic', it should return the same chip, if platdata is filled.
> + */
> +int pmic_io_dev(struct udevice *pmic, struct udevice **pmic_io);
> +
> +/**
> + * pmic_if_* functions: returns one of given pmic interface attribute:
> + * - type                         - one of enum { PMIC_I2C, PMIC_SPI, PMIC_NONE}
> + * - bus number                   - usually i2c/spi bus seq number
> + * - addres for i2c or cs for spi - basaed on dm_i2c/spi_chip
> + * - max offset                   - device internal address range
> + * - string - "I2C"/"SPI"/"--"
> + *
> + * @pmic - pointer to the pmic device
> + *
> + * Returns: one of listed attribute on success, or a negative value of errno.
> + */
> +int pmic_if_type(struct udevice *pmic);
> +int pmic_if_bus_num(struct udevice *pmic);
> +int pmic_if_addr_cs(struct udevice *pmic);
> +int pmic_if_max_offset(struct udevice *pmic);
> +const char *pmic_if_str(struct udevice *pmic);

I hope that none of these is needed in fact.

> +
> +/**
> + * pmic_read/write: read/write to the UCLASS_PMIC device
> + *
> + * The required pmic device can be obtained by:
> + * - pmic_i2c_get()
> + * - pmic_spi_get()
> + * - pmic_io_dev() - for pmic command purposes
> + *
> + * @pmic - pointer to the UCLASS_PMIC device
> + * @reg  - device register offset
> + * @val  - value for write or its pointer to read
> + *
> + * Returns: offset value on success or negative value of errno.

s/offset/register/ ?

> + */
> +int pmic_read(struct udevice *pmic, unsigned reg, unsigned char *val);
> +int pmic_write(struct udevice *pmic, unsigned reg, unsigned char val);
> +#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 +352,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